Re: [PATCH 1b/9] Tilera (and Linux asm-generic) support for glibc

[Chris Metcalf <cmetcalf at tilera dot com>]

On 11/4/2011 5:03 PM, Joseph S. Myers wrote:
> On Fri, 4 Nov 2011, Chris Metcalf wrote:
>
>> This patch is the disassembly support for Tile backtracing.  To be as
>> robust as possible, we include support for discovering the frame base
>> of the bottom-most stack frame by code inspection.  The files are mostly
>> tables that pack down well when compiled.  The same code is present in
>> the kernel to handle backtracing there.
> Could you explain how unwinding works on your processors that is different 
> from normal DWARF-2 unwinding (both .eh_frame and .debug_frame)

We developed our technique back when it was less common to have debug
information routinely loaded into memory along with every code section;
Tilera was founded back in 2005 and our backtrace/disassembly technique
dates back to then.  (I think the binutils we were using back then didn't
have CFI support in the assembler, for example.)  The advantage of the
disassembly approach, nominally, is that you can backtrace any code
produced by the compiler (or by "backtracer-compliant" assembly, a pretty
low bar), even if no .eh_frame is present.

The ABI documents are not very good on this score, you're right.  The ABI
document (UG213) mentions the unwind ABI a little bit (page 9) but there's
certainly more detail that we should include.

It's probably worth our looking into this further, so what I may do is pull
the disassembly support from the community return for now, and determine if
we like the DWARF/.eh_frame approach sufficiently at this point to adopt it
instead of our existing backtracer model.  The generic glibc backtracer
(non-IA64) works fine with an appropriate tile stack frame definition, as
long you are starting from a function with a stack frame.

The other thing that our backtracer does correctly is parse the kernel's
sigreturn data so that it can backtrace across a signal.  Does the IA64
DWARF-based backtracer handle that?

> +const struct tilegx_opcode tilegx_opcodes[334] =
> As far as I can see the name tilegx_opcodes is not used at all outside 
> this file, and so it should be static.

Your remaining comments in this email are all good (and uncontroversial);
the backtracer and disassembly code were borrowed into glibc from a
separate project within Tilera, so despite some code manipulation there
were a few issues of file-static scope, etc., that apparently I didn't get
quite right.  I'll review all of that stuff and make sure that it's correct
if we decide the right thing to do is to stay with the
disassembly+backtracer approach.

Thanks!

> * In general, if an object (.o) in glibc contains a symbol that is not 
> meant to be available outside that object (whether to other objects in 
> glibc or to the user of glibc) then it should be static.
>
> * If a symbol is an internal interface used by other objects within the 
> same shared library but not meant to be available to users of glibc or 
> other shared libraries within glibc, then it needs to be in the 
> implementation namespace (starting "__") and should preferably be hidden 
> visibility; this avoids interfering with the user's symbols when static 
> linking.
>
> * If a symbol is meant to be available to other shared libraries within 
> glibc, but not directly to the user, it needs to be in the implementation 
> namespace, symbol version GLIBC_PRIVATE, but not hidden visibility.
>
> * Finally, symbols available to the user through glibc's shared libraries 
> (including those in macro in glibc's installed headers, or used in objects 
> such as libc_nonshared.a that are statically linked into user programs and 
> shared libraries) need to be exported at symbol versions other than 
> GLIBC_PRIVATE.  But if they are only used from macros and libc_nonshared 
> rather than directly they should still be in the implementation namespace.
>
>> +const unsigned short * const
>> +tilegx_bundle_decoder_fsms[TILEGX_NUM_PIPELINE_ENCODINGS] =
> static.
>
>> +const struct tilegx_operand tilegx_operands[35] =
> static.
>
>> +int
>> +parse_insn_tilegx(tilegx_bundle_bits bits,
>> +                  unsigned long long pc,
>> +                  struct tilegx_decoded_instruction
>> +                  decoded[TILEGX_MAX_INSTRUCTIONS_PER_BUNDLE])
> Hidden, implementation namespace.
>
>> diff --git a/sysdeps/tile/tilepro/tile-desc.c b/sysdeps/tile/tilepro/tile-desc.c
>> new file mode 100644
>> index 0000000..1b066e5
>> --- /dev/null
>> +++ b/sysdeps/tile/tilepro/tile-desc.c
> Same comments apply here as well.
>
> The headers will also need corresponding updates.
-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com