Re: [PATCH] PR varobj/18564 regression in showing __thread so extern variable

[Philippe Waroquiers <philippe dot waroquiers at skynet dot be>]

On Wed, 2015-09-02 at 14:03 +0100, Yao Qi wrote:
> Philippe Waroquiers <philippe.waroquiers@skynet.be> writes:
> 
> Hi Philippe,
> I am not an expert on symbol stuff, but I can only review test case.
> Some one else may review the rest of the patch later.
Thanks for the review comments, find feedback below.
I have handled all comments (except one for which I have a question).
I will repost a V2 patch after review of the rest of the patch (i.e.
the bug fixing part).

> > diff --git a/gdb/testsuite/gdb.threads/tls-so_extern.c b/gdb/testsuite/gdb.threads/tls-so_extern.c
> We need a copy right header.
copyright header added
  (note that several existing files in gdb.threads
   have no copyright header, e.g. tls.c, tls-main.c, ...)

> 
> > diff --git a/gdb/testsuite/gdb.threads/tls-so_extern.exp b/gdb/testsuite/gdb.threads/tls-so_extern.exp
> Either 2014-2015 or 2015.
Changed to 2003-2015, as I started from tls-shared.exp


> > +remote_exec build "rm -f ${binfile}"
> Don't need to remove binfile.
I removed the remove :).
Note that this line originates from tls-shared.exp.

> > diff --git a/gdb/testsuite/gdb.threads/tls-so_extern_main.c b/gdb/testsuite/gdb.threads/tls-so_extern_main.c
> Copy right header is needed.
Copyright added.

> 
> > +#include <stdio.h>
> > +#include <pthread.h>
> > +
> > +extern __thread void *so_extern;
> > +
> > +static void *tls_ptr(void *p)
> 
> The code should comply to GNU coding standard.
Reformatted the code (e.g. space before (, function name
starting at begin of line, ...)

> 
> > +{
> > +   so_extern = &so_extern;
> > +   printf("address is %p\n", &so_extern); /* break here to check result */
> 
> Don't have to call printf and include stdio.h.
Not clear by what to replace this printf (or why it harms).

I need to put a break after the assignment, as the .exp 
will compare so_extern value with address of so_extern.
(note: I changed the .exp, so as to also check so_extern value
in main thread).
The printf line allows to put a break after the assignment.
If it is really better to remove the printf/stdio.h, any suggestion
about what to replace it with ? (we need to avoid the compiler to
optimise away this code to be sure we can put a break).

Thanks for the review/detailed comments,


Philippe