Re: [PATCH 3/3] Add test for fetching TLS from core file

[Pedro Alves <palves at redhat dot com>]

Hi Maciej,

Sorry, I hadn't noticed the question below directed at me.

On 10/18/2017 02:35 PM, Maciej W. Rozycki wrote:

>  Pedro, would you be able to comment on my observations and, 

They sound reasonable to me.

> ultimately 
> (once the details have been sorted out), approve this change (as 
> technically it's outside my area of competence anyway)?

Sure.

>> +}
>> +
>> +gdb_test "p/x foo" \
>> +                    "\\$\[0-9]+ = 0xdeadbeef" \
>> +                    "Printing thread-local storage variable."
> 
>  Formatting: use tabs rather than spaces to indent; one per line will do.
> 
>  Please wait a couple of days to give Pedro an opportunity to chime in and 
> then resubmit the whole series with the details above and 1/3 updated.
> 

There's code in the "thread" function that seemingly will never
be executed:

 +void *
 +thread (void *in) /* BREAK */
 +{
 +  int *tmp = (int *) in;
 +  int value = *tmp;
 +  foo += *tmp;
 +  while (1)
 +    sleep (10);
 +}

since there's a breakpoint set at "BREAK" and the testcase stops there
to generate a core with "gcore" as soon as the first thread
reaches the "BREAK" line.

But what's the point of that code in the "thread" function if
it's never going to be executed?  That only serves to obfuscate
as is.  I may well have missed something, of course.

 +gdb_test "p/x foo" \
 +                    "\\$\[0-9]+ = 0xdeadbeef" \
 +                    "Printing thread-local storage variable."

For test name/message, please write instead:

                 "print thread-local storage variable"

i.e., no uppercase, no period, and avoid gerund.

+gdb_exit
+
+#
+# Restart gdb and load generated corefile.
+#
+gdb_start
+gdb_reinitialize_dir $srcdir/$subdir
+gdb_load ${binfile}

This above can all be replaced by a call to clean_restart.

Thanks,
Pedro Alves