Re: [PATCH 5/9] Add a helper function to resolve TLS variable addresses for FreeBSD.

[John Baldwin <jhb at FreeBSD dot org>]

On 2/7/19 9:02 AM, John Baldwin wrote:
> On 2/6/19 9:04 PM, Simon Marchi wrote:
>> On 2019-01-24 12:08, John Baldwin wrote:
>>> On 1/22/19 10:42 AM, John Baldwin wrote:
>>>> The fbsd_get_thread_local_address function accepts the base address of
>>>> a thread's DTV array and the base address of an object file's link map
>>>> and uses this to compute a TLS variable's address.  FreeBSD
>>>> architectures use an architecture-specific method to determine the
>>>> address of the DTV array pointer and call this helper function to
>>>> perform the rest of the address calculation.
>>>>
>>>> 	* fbsd-tdep.c (fbsd_pspace_data_handle): New variable.
>>>> 	(struct fbsd_pspace_data): New type.
>>>> 	(get_fbsd_pspace_data, fbsd_pspace_data_cleanup)
>>>> 	(fbsd_read_integer_by_name, fbsd_fetch_rtld_offsets)
>>>> 	(fbsd_get_tls_index, fbsd_get_thread_local_address): New function.
>>>> 	(_initialize_fbsd_tdep): Initialize 'fbsd_pspace_data_handle'.
>>>> 	* fbsd-tdep.c (fbsd_get_thread_local_address): New prototype.
>>>> ---
>>>>  gdb/ChangeLog   |  10 ++++
>>>>  gdb/fbsd-tdep.c | 146 
>>>> ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  gdb/fbsd-tdep.h |  10 ++++
>>>>  3 files changed, 166 insertions(+)
>>>>
>>>> diff --git a/gdb/fbsd-tdep.c b/gdb/fbsd-tdep.c
>>>> index d971d3a653..2e0f72d17b 100644
>>>> --- a/gdb/fbsd-tdep.c
>>>> +++ b/gdb/fbsd-tdep.c
>>>> +
>>>> +/* Lookup offsets of fields in the runtime linker's 'Obj_Entry'
>>>> +   structure needed to determine the TLS index of an object file.  */
>>>> +
>>>> +static void
>>>> +fbsd_fetch_rtld_offsets (struct gdbarch *gdbarch, struct 
>>>> fbsd_pspace_data *data)
>>>> +{
>>>> +  TRY
>>>> +    {
>>>> +      /* Fetch offsets from debug symbols in rtld.  */
>>>> +      data->off_linkmap = parse_and_eval_long ("&((Obj_Entry 
>>>> *)0)->linkmap");
>>>> +      data->off_tlsindex = parse_and_eval_long ("&((Obj_Entry 
>>>> *)0)->tlsindex");
>>>> +      data->rtld_offsets_valid = true;
>>>> +      return;
>>>
>>> I'm not really happy about using parse_and_eval_long with an open-coded
>>> equivalent of offsetof() here.  It seems we don't already have existing
>>> functionality for this though?  I think I could use 'lookup_struct' to 
>>> find
>>> the 'struct type *' for 'Obj_Entry', but if I used 
>>> 'lookup_struct_elt_type'
>>> to get the type of an element that doesn't give me anything that has 
>>> the
>>> offset.  We could perhaps instead add a new function 
>>> 'lookup_struct_elt_offset'
>>> that took the element name as a string and figured out the offset.  We 
>>> could
>>> then use this to provide an 'offsetof' builtin for the C language 
>>> perhaps.
>>> However, I suspect that lookup_struct_elt_offset would have to invoke a
>>> language-specific function to do the actual computation (just as ptype 
>>> /o
>>> handling is language-specific).
>>
>> Doesn't parse_and_eval_long also call in language-specific things?  The 
>> expression is parsed according to whatever is the current language, I 
>> suppose.  If needed, I guess we could change temporarily the current 
>> language to C, which is the language the dynamic linker is written in 
>> (until proven otherwise).
> 
> Mmm, that's true.

I chose to use lookup_symbol_by_language to force C.

>> The offset of fields within structures is available (see macro 
>> FIELD_BITPOS).  I haven't looked too deep into it, but it sounds like it 
>> should be possible to implement what you need with that.
> 
> Hmm.  When I looked at the code for ptype /o it seemed to keep a running
> tally for the byte offset.  It wasn't clear to me if perhaps FIELD_BITPOS
> was for bitfields and the bit offset within a byte/word.  However, the
> Linux kernel patch series does have some code to do this.  I could perhaps
> steal from that to implement lookup_struct_elt_offset.  I would probably
> make it support nested fields as well (I think the Linux kernel patches are
> using it for some nested fields).

So this was actually simpler than I expected.  The existing
lookup_struct_elt_type didn't work because you can't work back from the
type to get to the field.  So instead I added a new function that returns
the field and the offset (you have to compute an offset still to handle
anonymous union/struct members) that is fairly similar to the lk_find_field
function in the Linux kernel patchset (and hopefully can be used in place of
that with minimal fuss).  It does return bitpositions that the consumer has
to convert to bytes, but that seems consistent and is easy to fix in the
consumer.  I then used this to replace the parse_and_eval_long above.  I'll
post a v3 with that.  I also reworded the commit message a bit for the debug
file patch you looked at and cleaned up some cruft from lookup_struct_elt_type.

Some other cleanup we might consider would be to reimplement the
lookup_struct_typedef in terms of lookup_struct (or vice versa), and to also
perhaps add a common version of the lk_find_type function from the Linux
kernel patchset that lets you name the structure type either by the tag
("foo" of "struct foo") or by a typedef ("foo_t" from
"typedef struct { } foo_t").

-- 
John Baldwin