Re: RFC: initial TLS patch

[Jim Blandy <jimb at redhat dot com>]

This was a very helpful review.  I'll revise the patch and re-post.
Specific comments below:

Andrew Cagney <ac131313@ges.redhat.com> writes:
> > +                   else if (is_thread_local)
> > +                     {
> > +                       SYMBOL_CLASS (sym) = LOC_THREAD_LOCAL_STATIC;
> > +                       SYMBOL_OBJFILE (sym) = objfile;
> > +                       SYMBOL_VALUE_ADDRESS (sym) = addr;
> > +                     }
> 
> Can I suggest awarding this mechanism a new LOC name.  That way the
> past history of LOC_THREAD_LOCAL_STATIC won't flow into this new
> mechanism. As for thread local static, looks like a separate pass can
> purge GDB of it.

I don't mind doing that.  But check out the comment in symtab.h:

    /* Value is at a thread-specific location calculated by a
       target-specific method. */

    LOC_THREAD_LOCAL_STATIC,

That's the pre-existing text.  It describes *exactly* what we're doing
here.  The only existing use of this address class, HP's, is actually
an abuse of this, because it implements this "target-specific method"
in non-target-specific code.  I would argue that I'm actually
correcting a broken situation: I'm making LOC_THREAD_LOCAL_STATIC mean
what it says it means.

I don't think the past history of LOC_THREAD_LOCAL_STATIC is a big
deal; If you grep for it through the current sources, it's pretty
obvious that the patch doesn't break anything.


> > !   TD_NOTALLOC	  /* TLS memory not yet allocated.  */
> 
> Suggest TLS -> Thread [Local] memory

Well, `thread-local storage' is the term used in all the ABI's and
compiler docs.  I think we should use the terms people are going to
see elsewhere.

> > + extern struct so_list *get_solib_by_objfile (struct objfile *OBJFILE);
> 
> Tipo OBJFILE -> objfile.

It was capitalized to match the comment above:

+ /* Return the so_list entry whose object file is OBJFILE.  */
+ extern struct so_list *get_solib_by_objfile (struct objfile *OBJFILE);
+ 

But the rest of GDB doesn't do that, so I'll change this.

> > Index: gdb/target.c
> > ===================================================================
> > RCS file: /cvs/src/src/gdb/target.c,v
> > retrieving revision 1.36
> > diff -c -r1.36 target.c
> > *** gdb/target.c	15 Jun 2002 21:07:58 -0000	1.36
> > --- gdb/target.c	29 Jun 2002 03:38:48 -0000
> > ***************
> > *** 610,615 ****
> > --- 610,616 ----
> >         INHERIT (to_async_mask_value, t);
> >         INHERIT (to_find_memory_regions, t);
> >         INHERIT (to_make_corefile_notes, t);
> > +       INHERIT (to_get_thread_local_value, t);
> >         INHERIT (to_magic, t);
> 
> Does this also need some debug stuff added?

Yes, thanks.


> >   #undef INHERIT
> > Index: gdb/target.h
> 
> > + +     /* Return the thread-local value of type TYPE at OFFSET in
> > the
> > +        thread-local storage for the thread PTID and the shared library
> > +        or executable file given by OBJFILE.  If that block of
> > +        thread-local storage hasn't been allocated yet, this function
> > +        may return an error, or try to concoct an appropriate
> > +        (non-lvalue) value based on the initialization image.  */
> > +     struct value *(*to_get_thread_local_value) (ptid_t PTID,
> > +                                                 struct objfile *OBJFILE,
> > +                                                 CORE_ADDR OFFSET,

I assume you want these in lower-case as well, right?

> Per other [discussion] thread.  I understand that at this time all
> that this needs to compute / return are: thread storage base address;
> conversion status - not allocated, readonly, read-write.  Having it
> return something more complicated like a ``struct value'' can be left
> to the person that actually needs the mechanism - I figure they will
> be in a better position to determine exactly what mechanism is needed.
> 
> Perhaphs there should be a separate ``struct location'' object?

Well, at the moment, the only extant implementation of this will
either return an address of the true value, or raise an error.  So
it's even simpler than that.  The prototype could simply be:

    CORE_ADDR (*to_get_thread_local_value) (ptid_t ptid,
                                            struct objfile *objfile,
                                            CORE_ADDR offset);

I agree that a `struct value' is more high-level than we'd really like
for a target operation.  Target methods should be messing with
CORE_ADDRs and sizes and machine-level stuff like that.

But the motivation here is to keep the interface innocent of the
different ways target-specific code might want to approximate the
"right" answer.  For example, since the thread-local storage
implementation at hand may not have the storage allocated yet, but
will have a read-only copy of the variable at the wrong address, an
interface like this would be fine:

    /* Return the address of PTID's thread-local storage for the load
       module (shared lib or main program) given by OBJFILE, at
       OFFSET, and set *ALLOCATED to 1.  If the thread-local storage
       has not been allocated, set *ALLOCATED to zero, and return the
       corresponding address in the initialization image, which
       contains the bits the variable *would* have if it *were*
       allocated.  Raise an error if we can't find the thread-local
       storage or the initialization image.  */
    CORE_ADDR (*to_get_thread_local_value) (ptid_t ptid,
                                            struct objfile *objfile,
                                            CORE_ADDR offset,
                                            int *allocated);

This is fine, and it covers everything that the IA-32 Linux TLS
implementation will do.  No problems here.

But, to me, it seems like this interface explicitly reflects the
quirks of the TLS implementation.  What if some other TLS
implementation requires, say, relocs to be applied to the
initialization image?  What if some processor with lots of registers
puts small TLS variables in registers?  (You could have register-sized
relocs, and let the static linker assign the register number.
Dynamically linked code couldn't do this, but that's okay.)

Maybe that's contrived.  But given how hairy TLS seems to be, I expect
to see some variety in the implementations.  And each time we
encounter another variant, then this interface will need to again be
expanded to accomodate that.  This target method will end up showing
every possible way anyone has ever constructed a thread-local value.

The motivation for returning a `struct value' is that we know the
interface will be adequate for any future TLS system, no matter what
tricks it wants to play.  We know the end result has to be a `struct
value' anyway in GDB, so that interface places no more constraints on
GDB's TLS support than the rest of GDB does.

So, although I do share folks' intuition that `struct value' is too
high-level for the target interface, it seems like a really nice
solution to the problem.

When I spoke with Michael S. on the phone, he expressed concern that
we were spreading knowledge about the internals of `struct value' into
areas of GDB that shouldn't know about it.  But I don't think that's
quite so.  Check out the code in thread_db_get_thread_local_value that
constructs the value.  It's trivial:

      return value_at_lazy (type, (CORE_ADDR) address, 0);

It constructs the value through clean, simple public interfaces.  And
we can add more interfaces to value.h if we need them.

In this approach, it is certainly the case that the target method
becomes aware of the different ways one might construct `struct value'
objects: lazy objects, non-lvalue, etc.  But the alternative is for
the get_thread_local_value method's interface to reflect all the
different states TLS might be in.  You've got complexity exported
across an interface either way --- but if let the TLS code return a
`struct value' it's not new complexity.


> > ***************
> > *** 1021,1026 ****
> > --- 1038,1050 ----
> >     #define target_make_corefile_notes(BFD, SIZE_P) \
> >        (current_target.to_make_corefile_notes) (BFD, SIZE_P)
> > + + + /* Thread-local values.  */
> > + #define target_get_thread_local_value \
> > +     (current_target.to_get_thread_local_value)
> > + #define target_has_get_thread_local_value() \
> > +     (target_get_thread_local_value != 0)
> 
> ``_has_'' in the target vector appears to be used when testing
> attributes such as ``the target has memory'' so
> target_get_thread_local_value_p() would be better.  Suggest ``!=
> NULL''.

Okay, I wasn't sure what was right here --- I'll fix this.

By the way --- won't the presence of the debug method break this test?
That is, if one enables debugging, then
`target_has_get_thread_local_value' will return true, even when it
doesn't.  Should there be a separate `int' member of the target saying
whether it supports `get_thread_local_value'?

> > +       if (! so)
> > +         error ((objfile_is_library
> > +                 ? ("Cannot find shared library `%s' in dynamic linker's "
> > +                    "module list")
> > +                 : ("Cannot find executable file `%s' in dynamic linker's "
> > +                    "module list")),
> > +                objfile->name);
> 
> Can I suggest writing these as:
> 	if (objfile_is_library)
> 	  error (...)
> 	else
> 	  error (...)
> it is going to make checking and for correct i18n much easier.  All
> errors will match:
> 	error[[:space:]]\(_\"[A-Z]

Sure thing.  I was trying to make this i18n-friendly by not
substituting words into the middle of the sentence, but I'll do
whatever the i18n person wants.

> > + #ifdef THREAD_DB_DEFINES_TD_NOTALLOC
> 
> I suspect you mean THREAD_DB_HAS_TD_NOTALLOC, if it were a #define,
> #ifdef TD_NOTALLOC would work :-)

Yes, thanks.