[PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Djordje Todorovic]

Hi Maciej,

>If your mail client makes it difficult to send text verbatim, then can 
>you please try a different solution, such as using `git send-email'?  

Sure, I will send all patches by using `git send-email'.  Please notice that I
have addressed all of your proposals.


>This patch series addresses problems with writing Linux core PRSTATUS note
>into MIPS o32, n32 and n64 core files.
>Also, there is one patch which adds reading PID information from MIPS o32, n32
>and n64 core file.

>> Great, thanks for addressing this part as well!

Thanks!

>I have tested it on MIPS64R2 board with ‘mips-mti-linux-gnu’ target and it is
>also tested with ‘x86_64-linux-gnu’ target and these changes have passed
>binutils regression tests.

>>Thanks.  Which ABI(s) specifically did you test for?

The changes have passed regression tests for MIPS32 (o32) target.

Thanks,
Djordje

Re: [PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Maciej W. Rozycki]

Hi Djordje,

> >If your mail client makes it difficult to send text verbatim, then can 
> >you please try a different solution, such as using `git send-email'?  
> 
> Sure, I will send all patches by using `git send-email'.  Please notice that I
> have addressed all of your proposals.

 Great, thanks!

> >I have tested it on MIPS64R2 board with ‘mips-mti-linux-gnu’ target and it is
> >also tested with ‘x86_64-linux-gnu’ target and these changes have passed
> >binutils regression tests.
> 
> >>Thanks.  Which ABI(s) specifically did you test for?
> 
> The changes have passed regression tests for MIPS32 (o32) target.

 Native presumably, right?  I'll push them through n32 and n64 native 
testing for you then and just smoke-test your newly added case with a 
cross-debugger and a remote target (where no core file generation is 
currently supported).

  Maciej

Re: [PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Djordje Todorovic]

Hi Maciej,

>Native presumably, right? I'll push them through n32 and n64 native
>testing for you then and just smoke-test your newly added case with a
>cross-debugger and a remote target (where no core file generation is
>currently supported).

Yes, native. Thanks a lot!

Thanks,
Djordje

Re: [PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Djordje Todorovic]

Hi Maciej,

Please notice that I have reformatted patches 1/3 and 3/3, according your comments, and also tweaked a little bit patch 3/3.

Regarding patch 3/3, as Pedro said, currently “thread” function has a piece of code that is never going to be executed since core file is going to be generated as soon as the first thread reaches the thread function.  So, since we are testing fetching value of TLS variable from core file, creating just a one thread in test case would be enough and regarding that, I have cut unnecessary code from the function.

Thanks,
Djordje

Re: [PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Maciej W. Rozycki]

Hi Djordje (and Pedro, see below),

> Please notice that I have reformatted patches 1/3 and 3/3, according 
> your comments, and also tweaked a little bit patch 3/3.

 Thank you.  There's still a small nit remaining WRT 1/3, but as I have 
noted in a separate reply I'll handle it myself when committing.

> Regarding patch 3/3, as Pedro said, currently “thread” function has a 
> piece of code that is never going to be executed since core file is 
> going to be generated as soon as the first thread reaches the thread 
> function.  So, since we are testing fetching value of TLS variable from 
> core file, creating just a one thread in test case would be enough and 
> regarding that, I have cut unnecessary code from the function.

 Hmm, to understand how exactly the bug triggers (as you may recall long 
ago I observed GDB prefers LWPID over PID if available) I ran your test 
case with the code changes removed and the test has still passed across 
the three ABIs.  So it does not actually cover the case concerned here.  
I still would like to keep it to verify the consistency between core files 
written and read (following the various issues discovered in the course of 
this review, including the n32 BFD fix I have cc-ed you on lately), 
however that brings the question again about how you originally observed 
the problem you've addressed.  Is there another test scenario that can be 
created?

 Also now that the issue of using `run' has been resolved, I have verified 
the test case with a cross-debugger against a remote target and the result 
is a FAIL like below:

(gdb) file .../gdb/testsuite/outputs/gdb.threads/tls-core/tls-core
Reading symbols from .../gdb/testsuite/outputs/gdb.threads/tls-core/tls-core...done.
(gdb) core .../gdb/testsuite/outputs/gdb.threads/tls-core/gcore.test
warning: core file may not match specified executable file.
[New LWP 8829]
[New LWP 8828]
Core was generated by `.../gdb/testsuite/output'.
Program terminated with signal SIGTRAP, Trace/breakpoint trap.
#0  thread_proc (arg=0x0) at .../gdb/testsuite/gdb.threads/tls-core.c:25
25        return arg;
[Current thread is 1 (LWP 8829)]
(gdb) PASS: gdb.threads/tls-core.exp: load generated corefile
p/x foo
Cannot find thread-local storage for LWP 8829, executable file .../gdb/testsuite/outputs/gdb.threads/tls-core/tls-core:
Cannot find thread-local variables on this target
(gdb) FAIL: gdb.threads/tls-core.exp: print thread-local storage variable

which I am fairly sure that is related (along with the "core file may not 
match specified executable file." message) to an attempt to use native 
`libthread_db'.

 Pedro, do I remember correctly it was you who recently mentioned (maybe 
at the Cauldron) the need to have the issue of using `libthread_db' in 
cross-debugging correctly addressed?  For the time being I think we want 
to KFAIL this test case if [is_remote target] (in its recently cleaned-up 
meaning, as the test case actually succeeds with `native-gdbserver') -- 
but do we have a tracking bug already?

  Maciej

Re: [PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Djordje Todorovic]

Hi Maciej,

>  Thank you.  There's still a small nit remaining WRT 1/3, but as I have
> noted in a separate reply I'll handle it myself when committing.

Thanks a lot! I really appreciate it!

>> Regarding patch 3/3, as Pedro said, currently “thread” function has a 
>> piece of code that is never going to be executed since core file is 
>> going to be generated as soon as the first thread reaches the thread 
>> function.  So, since we are testing fetching value of TLS variable from 
>> core file, creating just a one thread in test case would be enough and 
>> regarding that, I have cut unnecessary code from the function.
>
>  Hmm, to understand how exactly the bug triggers (as you may recall long
> ago I observed GDB prefers LWPID over PID if available) I ran your test
> case with the code changes removed and the test has still passed across
> the three ABIs.  So it does not actually cover the case concerned here.  
> I still would like to keep it to verify the consistency between core files
> written and read (following the various issues discovered in the course of
> this review, including the n32 BFD fix I have cc-ed you on lately),
> however that brings the question again about how you originally observed
> the problem you've addressed.  Is there another test scenario that can be
> created?

Thanks a lot, I saw that n32 BFD fix, and it is great!

Actually, I have faced it when I tried to read value of TLS variable on MIPS platforms (native).  I agree with you that GDB prefers LWPID over PID if available, but with no PID from core file GDB, at least on my boards, can not read value of TLS variable.  I have tried to do some observation on x86_64 native platform and commented piece of code for fetching PID from core file like this:

diff --git a/bfd/elf64-x86-64.c b/bfd/elf64-x86-64.c
index 6a5159d..9ee9231 100644
--- a/bfd/elf64-x86-64.c
+++ b/bfd/elf64-x86-64.c
@@ -415,8 +415,8 @@ elf_x86_64_grok_psinfo (bfd *abfd, Elf_Internal_Note *note)
        break;

      case 136:                /* sizeof(struct elf_prpsinfo) on Linux/x86_64 */
-       elf_tdata (abfd)->core->pid
-         = bfd_get_32 (abfd, note->descdata + 24);
+       //elf_tdata (abfd)->core->pid
+       //  = bfd_get_32 (abfd, note->descdata + 24);
        elf_tdata (abfd)->core->program
         = _bfd_elfcore_strndup (abfd, note->descdata + 40, 16);
        elf_tdata (abfd)->core->command
 
and rebuilt GDB (with "make clean" and "make"), the test case was FAIL:

(gdb) file /home/rtrk/gdb/build_native/gdb/testsuite/outputs/gdb.threads/tls-core/tls-core
Reading symbols from /home/rtrk/gdb/build_native/gdb/testsuite/outputs/gdb.threads/tls-core/tls-core...done.
(gdb) core /home/rtrk/gdb/build_native/gdb/testsuite/outputs/gdb.threads/tls-core/gcore.test
[New LWP 12556]
[New LWP 12552]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `/home/rtrk/gdb/build_native/gdb/testsuite/outputs/gdb.threads/tls-core/tls-core'.
Program terminated with signal SIGTRAP, Trace/breakpoint trap.
#0 thread_proc (arg=0x0) at /home/rtrk/gdb/build_native/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.threads/tls-core.c:25
25 return arg;
[Current thread is 1 (LWP 12556)]
(gdb) PASS: gdb.threads/tls-core.exp: load generated corefile
p/x foo
Cannot find user-level thread for LWP 12556: generic error

When I reverted this observation changes and rebuilt it again, GDB was able to read value of TLS variable:

(gdb) file /home/rtrk/gdb/build_native/gdb/testsuite/outputs/gdb.threads/tls-core/tls-core
Reading symbols from /home/rtrk/gdb/build_native/gdb/testsuite/outputs/gdb.threads/tls-core/tls-core...done.
(gdb) core /home/rtrk/gdb/build_native/gdb/testsuite/outputs/gdb.threads/tls-core/gcore.test
[New LWP 11099]
[New LWP 11095]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
Core was generated by `/home/rtrk/gdb/build_native/gdb/testsuite/outputs/gdb.threads/tls-core/tls-core'.
Program terminated with signal SIGTRAP, Trace/breakpoint trap.
#0 thread_proc (arg=0x0) at /home/rtrk/gdb/build_native/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.threads/tls-core.c:25
25 return arg;
[Current thread is 1 (Thread 0x7ffff77ef700 (LWP 11099))]
(gdb) PASS: gdb.threads/tls-core.exp: load generated corefile
p/x foo
$1 = 0xdeadbeef

Exactly the same situation I have reproduced on MIPS platforms. Have I missed something?


Regarding fetching value of TLS variable with cross or Multiarch GDB, we actually had proposal for resolving that issue in GDB, where we proposed that GDB can have its own "libthread_db" functions for getting TLS in case of cross debugging, where it is going to "simulate" "target" architecture, but that code would be hard to maintain, because every changes in "libthread_db" functions regarding particular architecture in GLIBC would take also changes in GDB, so it was not accepted by community, reasonably (please briefly see that thread: http://sourceware-org.1504.n7.nabble.com/PATCH-TLS-access-support-in-cross-Linux-GDB-td452155.html). But yes, I also think in those cases, test should be marked as known failure.

Thanks,
Djordje

Re: [PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Pedro Alves]

On 10/25/2017 11:38 PM, Maciej W. Rozycki wrote:

> (gdb) PASS: gdb.threads/tls-core.exp: load generated corefile
> p/x foo
> Cannot find thread-local storage for LWP 8829, executable file .../gdb/testsuite/outputs/gdb.threads/tls-core/tls-core:
> Cannot find thread-local variables on this target
> (gdb) FAIL: gdb.threads/tls-core.exp: print thread-local storage variable
> 
> which I am fairly sure that is related (along with the "core file may not 
> match specified executable file." message) to an attempt to use native 
> `libthread_db'.

The "core file may not match specified executable file." sounds like
something else.  I think you'll get that if the program name recorded
in the core data structure is not the same as the name of the executable.
Try setting a breakpoint on core_file_matches_executable_p.

> 
>  Pedro, do I remember correctly it was you who recently mentioned (maybe 
> at the Cauldron) the need to have the issue of using `libthread_db' in 
> cross-debugging correctly addressed?  For the time being I think we want 
> to KFAIL this test case if [is_remote target] (in its recently cleaned-up 
> meaning, as the test case actually succeeds with `native-gdbserver') -- 
> but do we have a tracking bug already?

Right, libthread_db is loaded by the host gdb, and that libthread_db
is only good for native debugging.  Things work with live cross-debugging 
against gdbserver because in that case it's gdbserver that loads
the right libthread_db, not gdb.

Thanks,
Pedro Alves

Re: [PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Maciej W. Rozycki]

On Fri, 27 Oct 2017, Pedro Alves wrote:

> > (gdb) PASS: gdb.threads/tls-core.exp: load generated corefile
> > p/x foo
> > Cannot find thread-local storage for LWP 8829, executable file .../gdb/testsuite/outputs/gdb.threads/tls-core/tls-core:
> > Cannot find thread-local variables on this target
> > (gdb) FAIL: gdb.threads/tls-core.exp: print thread-local storage variable
> > 
> > which I am fairly sure that is related (along with the "core file may not 
> > match specified executable file." message) to an attempt to use native 
> > `libthread_db'.
> 
> The "core file may not match specified executable file." sounds like
> something else.  I think you'll get that if the program name recorded
> in the core data structure is not the same as the name of the executable.

 Given that this is not the case here, as the basename of the program is 
supposed to be `tls-core' and that fits in the (regrettable) 16-character 
limit for the executable name in Linux core files, there must be something 
odd going on here.

> Try setting a breakpoint on core_file_matches_executable_p.

 I will, thanks for the hint.

  Maciej

Re: [PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Maciej W. Rozycki]

Hi Djordje,

> >  Hmm, to understand how exactly the bug triggers (as you may recall long
> > ago I observed GDB prefers LWPID over PID if available) I ran your test
> > case with the code changes removed and the test has still passed across
> > the three ABIs.  So it does not actually cover the case concerned here.  
> > I still would like to keep it to verify the consistency between core files
> > written and read (following the various issues discovered in the course of
> > this review, including the n32 BFD fix I have cc-ed you on lately),
> > however that brings the question again about how you originally observed
> > the problem you've addressed.  Is there another test scenario that can be
> > created?
> 
> Thanks a lot, I saw that n32 BFD fix, and it is great!
> 
> Actually, I have faced it when I tried to read value of TLS variable on MIPS platforms (native).  I agree with you that GDB prefers LWPID over PID if available, but with no PID from core file GDB, at least on my boards, can not read value of TLS variable.  I have tried to do some observation on x86_64 native platform and commented piece of code for fetching PID from core file like this:
> 
> diff --git a/bfd/elf64-x86-64.c b/bfd/elf64-x86-64.c
> index 6a5159d..9ee9231 100644
> --- a/bfd/elf64-x86-64.c
> +++ b/bfd/elf64-x86-64.c
> @@ -415,8 +415,8 @@ elf_x86_64_grok_psinfo (bfd *abfd, Elf_Internal_Note *note)
>         break;
> 
>       case 136:                /* sizeof(struct elf_prpsinfo) on Linux/x86_64 */
> -       elf_tdata (abfd)->core->pid
> -         = bfd_get_32 (abfd, note->descdata + 24);
> +       //elf_tdata (abfd)->core->pid
> +       //  = bfd_get_32 (abfd, note->descdata + 24);
>         elf_tdata (abfd)->core->program
>          = _bfd_elfcore_strndup (abfd, note->descdata + 40, 16);
>         elf_tdata (abfd)->core->command
>  
> and rebuilt GDB (with "make clean" and "make"), the test case was FAIL:
> 
> (gdb) file /home/rtrk/gdb/build_native/gdb/testsuite/outputs/gdb.threads/tls-core/tls-core
> Reading symbols from /home/rtrk/gdb/build_native/gdb/testsuite/outputs/gdb.threads/tls-core/tls-core...done.
> (gdb) core /home/rtrk/gdb/build_native/gdb/testsuite/outputs/gdb.threads/tls-core/gcore.test
> [New LWP 12556]
> [New LWP 12552]
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> Core was generated by `/home/rtrk/gdb/build_native/gdb/testsuite/outputs/gdb.threads/tls-core/tls-core'.
> Program terminated with signal SIGTRAP, Trace/breakpoint trap.
> #0 thread_proc (arg=0x0) at /home/rtrk/gdb/build_native/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.threads/tls-core.c:25
> 25 return arg;
> [Current thread is 1 (LWP 12556)]
> (gdb) PASS: gdb.threads/tls-core.exp: load generated corefile
> p/x foo
> Cannot find user-level thread for LWP 12556: generic error
> 
> When I reverted this observation changes and rebuilt it again, GDB was 
> able to read value of TLS variable:
> 
> (gdb) file /home/rtrk/gdb/build_native/gdb/testsuite/outputs/gdb.threads/tls-core/tls-core
> Reading symbols from /home/rtrk/gdb/build_native/gdb/testsuite/outputs/gdb.threads/tls-core/tls-core...done.
> (gdb) core /home/rtrk/gdb/build_native/gdb/testsuite/outputs/gdb.threads/tls-core/gcore.test
> [New LWP 11099]
> [New LWP 11095]
> [Thread debugging using libthread_db enabled]
> Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
> Core was generated by `/home/rtrk/gdb/build_native/gdb/testsuite/outputs/gdb.threads/tls-core/tls-core'.
> Program terminated with signal SIGTRAP, Trace/breakpoint trap.
> #0 thread_proc (arg=0x0) at /home/rtrk/gdb/build_native/gdb/testsuite/../../../binutils-gdb/gdb/testsuite/gdb.threads/tls-core.c:25
> 25 return arg;
> [Current thread is 1 (Thread 0x7ffff77ef700 (LWP 11099))]
> (gdb) PASS: gdb.threads/tls-core.exp: load generated corefile
> p/x foo
> $1 = 0xdeadbeef
> 
> Exactly the same situation I have reproduced on MIPS platforms. Have I 
> missed something?

 I'll see if I can reproduce it with the x86-64 target and that may help 
me understand why I don't see it with the MIPS one.

> Regarding fetching value of TLS variable with cross or Multiarch GDB, we 
> actually had proposal for resolving that issue in GDB, where we proposed 
> that GDB can have its own "libthread_db" functions for getting TLS in 
> case of cross debugging, where it is going to "simulate" "target" 
> architecture, but that code would be hard to maintain, because every 
> changes in "libthread_db" functions regarding particular architecture in 
> GLIBC would take also changes in GDB, so it was not accepted by 
> community, reasonably (please briefly see that thread: 
> http://sourceware-org.1504.n7.nabble.com/PATCH-TLS-access-support-in-cross-Linux-GDB-td452155.html). 
> But yes, I also think in those cases, test should be marked as known 
> failure.

 Thank you for the reference, and indeed in this case, with the problem 
being dealt with already, I'm even more confident that having it recorded 
as a known failure is the right way.  But that needs an existing issue 
report in our Bugzilla to refer to.  As you have been handling this 
already can you file one please?

 Once we have an issue number, we can then:

    if [is_remote target] {
	setup_kfail "gdb/..." "*-*-*"
    }

ahead of the offending case.  Overall we must never gratuitously cause new 
FAILs to appear in testing with new test cases -- either the causing issue 
should be fixed, or the problem KFAILed or XFAILed as appropriate, 
depending on the circumstances.

 I'm off throughout this week and this is my last reply in this thread 
until I am back.  Can you please use the time to enter the bug report, and 
can you also tell me if your copyright assignment has been sorted out with 
FSF?

  Maciej

Re: [PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Maciej W. Rozycki]

On Mon, 30 Oct 2017, Maciej W. Rozycki wrote:

>  Once we have an issue number, we can then:
> 
>     if [is_remote target] {
> 	setup_kfail "gdb/..." "*-*-*"
>     }
> 
> ahead of the offending case.

 Or:

    if {![string match $host_triplet $target_triplet]} {
	setup_kfail "gdb/..." "*-*-*"
    }

really (`isnative' doesn't help here as it's too restrictive: we could 
have a native GDB built on a different system; this is actually what I use 
as I don't currently have an adequate version of native MIPS GCC set up).

  Maciej

Re: [PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Djordje Todorovic]

Hi Maciej,

>Can you please use the time to enter the bug report, and 
>can you also tell me if your copyright assignment has been sorted out with 
>FSF?

The bug report is created and it can be found on the following link:
https://sourceware.org/bugzilla/show_bug.cgi?id=22381

Also, the copyright assignment with FSF has been sorted out.

>    if {![string match $host_triplet $target_triplet]} {
>	setup_kfail "gdb/..." "*-*-*"
>    }
>
>really (`isnative' doesn't help here as it's too restrictive: we could 
>have a native GDB built on a different system; this is actually what I use 
>as I don't currently have an adequate version of native MIPS GCC set up).

I agree with you that this is more appropriate way to do it.

So, could you please, tell me do you want from me to resubmit the whole patch series
or just the last one?

Thanks,
Djordje

Re: [PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Maciej W. Rozycki]

On Mon, 30 Oct 2017, Maciej W. Rozycki wrote:

> > Exactly the same situation I have reproduced on MIPS platforms. Have I 
> > missed something?
> 
>  I'll see if I can reproduce it with the x86-64 target and that may help 
> me understand why I don't see it with the MIPS one.

 So I have looked into it now and tracked down `libthread_db' rather 
than GDB to be the component requiring PID retrieval from a core file 
for TLS access to work, and then only before glibc commit c579f48edba8 
("Remove cached PID/TID in clone"), which was first included in 2.25 
glibc release.

 Given I have been using recent glibc checkouts for MIPS verification I 
avoided the requirement, but I was indeed able to reproduce it natively 
with x86-64 and the system-supplied `libthread_db', and then with my 
MIPS environment as well, once I went with my glibc build back to commit 
c579f48edba8^.

 I will be adding a reference to said glibc commit when pushing your 2/3 
change.

  Maciej

Re: [PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Maciej W. Rozycki]

Hi Djordje,

> >Can you please use the time to enter the bug report, and 
> >can you also tell me if your copyright assignment has been sorted out with 
> >FSF?
> 
> The bug report is created and it can be found on the following link:
> https://sourceware.org/bugzilla/show_bug.cgi?id=22381

 Thank you, I have updated it with some pointers, based on your earlier 
reference.

> Also, the copyright assignment with FSF has been sorted out.

 Great!

> >    if {![string match $host_triplet $target_triplet]} {
> >	setup_kfail "gdb/..." "*-*-*"
> >    }
> >
> >really (`isnative' doesn't help here as it's too restrictive: we could 
> >have a native GDB built on a different system; this is actually what I use 
> >as I don't currently have an adequate version of native MIPS GCC set up).
> 
> I agree with you that this is more appropriate way to do it.
> 
> So, could you please, tell me do you want from me to resubmit the whole patch series
> or just the last one?

 I think there is no point in resending, as long as Pedro is fine with 
and approves your most recent version of the test case patch posted (NB 
it helps if on resubmission you number patch revisions in the subject 
line like [PATCH v3 2/3]).  Pedro?

 Once we've got the final ack from Pedro, I'll proceed with the right 
steps to push your changes, with the tweaks noted in the final review 
round applied.  I'll post the actual patches committed, for posterity.

  Maciej

Re: [PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Pedro Alves]

On 11/07/2017 09:29 PM, Maciej W. Rozycki wrote:
> On Mon, 30 Oct 2017, Maciej W. Rozycki wrote:
> 
>>> Exactly the same situation I have reproduced on MIPS platforms. Have I 
>>> missed something?
>>
>>  I'll see if I can reproduce it with the x86-64 target and that may help 
>> me understand why I don't see it with the MIPS one.
> 
>  So I have looked into it now and tracked down `libthread_db' rather 
> than GDB to be the component requiring PID retrieval from a core file 
> for TLS access to work, and then only before glibc commit c579f48edba8 
> ("Remove cached PID/TID in clone"), which was first included in 2.25 
> glibc release.
> 
>  Given I have been using recent glibc checkouts for MIPS verification I 
> avoided the requirement, but I was indeed able to reproduce it natively 
> with x86-64 and the system-supplied `libthread_db', and then with my 
> MIPS environment as well, once I went with my glibc build back to commit 
> c579f48edba8^.
> 
>  I will be adding a reference to said glibc commit when pushing your 2/3 
> change.

Interesting, hadn't realized libthread_db stopped requiring this.
I wonder whether it'd be a good idea to mention it in the code itself 
too, say, in proc-service.:ps_getpid.

Thanks,
Pedro Alves

Re: [PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Pedro Alves]

On 11/07/2017 09:58 PM, Maciej W. Rozycki wrote:

>> So, could you please, tell me do you want from me to resubmit the whole patch series
>> or just the last one?
> 
>  I think there is no point in resending, as long as Pedro is fine with 
> and approves your most recent version of the test case patch posted 

...

(NB it helps if on resubmission you number patch revisions in the subject
> line like [PATCH v3 2/3]).  Pedro?

Agreed.  And threading the patch series emails under the cover letter
also helps a lot.  In fact, I'm a bit lost and am not sure which is the
last version of the test patch that I should be looking at. :-P  :-)

I think... this one:
  https://sourceware.org/ml/gdb-patches/2017-10/msg00770.html
?

That looks fine to me.

> 
>  Once we've got the final ack from Pedro, I'll proceed with the right 
> steps to push your changes, with the tweaks noted in the final review 
> round applied.  I'll post the actual patches committed, for posterity.

Thanks,
Pedro Alves

Re: [PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Maciej W. Rozycki]

On Wed, 8 Nov 2017, Pedro Alves wrote:

> >  So I have looked into it now and tracked down `libthread_db' rather 
> > than GDB to be the component requiring PID retrieval from a core file 
> > for TLS access to work, and then only before glibc commit c579f48edba8 
> > ("Remove cached PID/TID in clone"), which was first included in 2.25 
> > glibc release.
> > 
> >  Given I have been using recent glibc checkouts for MIPS verification I 
> > avoided the requirement, but I was indeed able to reproduce it natively 
> > with x86-64 and the system-supplied `libthread_db', and then with my 
> > MIPS environment as well, once I went with my glibc build back to commit 
> > c579f48edba8^.
> > 
> >  I will be adding a reference to said glibc commit when pushing your 2/3 
> > change.
> 
> Interesting, hadn't realized libthread_db stopped requiring this.
> I wonder whether it'd be a good idea to mention it in the code itself 
> too, say, in proc-service.:ps_getpid.

 This might not be the best place.  Calls to `ps_getpid' are still made 
from places throughout `libthread_db', and it's only one from 
`td_ta_thr_iter' (which matters here) whose result is discarded, by only 
passing it down to `iterate_thread_list' to become its ignored 
`match_pid' argument.  This looks like an oversight of some kind to me, 
which Adhemerval might be able to shed some light on.

 Adhemerval, in your commit referred above, corresponding to the change 
held at: <https://sourceware.org/ml/libc-alpha/2016-11/msg00282.html>, 
you made the `match_pid' argument of `iterate_thread_list' unused.  The 
function is static, called from `td_ta_thr_iter' only (and I actually 
wonder why the build does not trip on an unused argument here).

 Is it OK then to remove the argument then along with the `ps_getpid' 
call in `td_ta_thr_iter' used to set the corresponding parameter, or is 
there something missing from there you intended to do and `match_pid' 
was meant to remain used?

 NB I would post a clean-up proposal, but regrettably my company FSF 
copyright assignment status is in a limbo state right now, after the 
transition from Imagination to the new MIPS company, and I'd rather not 
block the cleanup by posting a patch that cannot move forward.  Or would 
it be small enough to qualify as not needing an assignment?  Hmm...

 As to choosing the right place to comment on this `libthread_db' 
semantics change, I think either the call to `bfd_core_file_pid' made 
from `add_to_thread_list' in corelow.c or the heading of the latter 
function might be better, as this is where the consumer of BFD's core 
file data PID is.  Adding a comment like this might be small/trivial 
enough for me to sneak in while the FSF paperwork is still pending.

  Maciej

Re: [PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Maciej W. Rozycki]

On Wed, 8 Nov 2017, Pedro Alves wrote:

> >  I think there is no point in resending, as long as Pedro is fine with 
> > and approves your most recent version of the test case patch posted 
> 
> ...
> 
> (NB it helps if on resubmission you number patch revisions in the subject
> > line like [PATCH v3 2/3]).  Pedro?
> 
> Agreed.  And threading the patch series emails under the cover letter
> also helps a lot.  In fact, I'm a bit lost and am not sure which is the
> last version of the test patch that I should be looking at. :-P  :-)
> 
> I think... this one:
>   https://sourceware.org/ml/gdb-patches/2017-10/msg00770.html
> ?

 Yes.  Apologies for not pointing you explicitly at it.

> That looks fine to me.

 Great.  In reply to this message I'm following up with the final 
versions committed.  These are v7 according to my notes.

  Maciej

[committed v7 2/3] BFD: Extract PID from MIPS core dump file

[Maciej W. Rozycki]

From: Djordje Todorovic <djordje.todorovic@rt-rk.com>

On MIPS o32, n32 and n64 platforms, PID information was not
correctly propagated from core dump file to internal GDB
structures.  This patch fixes that behavior.

A correct PID is needed by `libthread_db' library supplied with
glibc repository revisions before commit c579f48edba8 ("Remove
cached PID/TID in clone") or released versions before 2.25 for
GDB to fetch value of TLS variable from core file.

bfd/ChangeLog:

	* elf32-mips.c (elf32_mips_grok_psinfo): Extract core->pid.
	* elf64-mips.c (elf64_mips_grok_psinfo): Likewise.
	* elfn32-mips.c (elf32_mips_grok_psinfo): Likewise.
---
 bfd/elf32-mips.c  |    2 ++
 bfd/elf64-mips.c  |    2 ++
 bfd/elfn32-mips.c |    2 ++
 3 files changed, 6 insertions(+)

Index: binutils/bfd/elf32-mips.c
===================================================================
--- binutils.orig/bfd/elf32-mips.c	2017-11-08 21:05:36.456371632 +0000
+++ binutils/bfd/elf32-mips.c	2017-11-08 21:05:37.530049782 +0000
@@ -2353,6 +2353,8 @@ elf32_mips_grok_psinfo (bfd *abfd, Elf_I
 	return FALSE;
 
       case 128:		/* Linux/MIPS elf_prpsinfo */
+	elf_tdata (abfd)->core->pid
+	 = bfd_get_32 (abfd, note->descdata + 16);
 	elf_tdata (abfd)->core->program
 	 = _bfd_elfcore_strndup (abfd, note->descdata + 32, 16);
 	elf_tdata (abfd)->core->command
Index: binutils/bfd/elf64-mips.c
===================================================================
--- binutils.orig/bfd/elf64-mips.c	2017-11-08 21:05:36.474538851 +0000
+++ binutils/bfd/elf64-mips.c	2017-11-08 21:05:37.538147336 +0000
@@ -4228,6 +4228,8 @@ elf64_mips_grok_psinfo (bfd *abfd, Elf_I
 	return FALSE;
 
       case 136:		/* Linux/MIPS - N64 kernel elf_prpsinfo */
+	elf_tdata (abfd)->core->pid
+	 = bfd_get_32 (abfd, note->descdata + 24);
 	elf_tdata (abfd)->core->program
 	 = _bfd_elfcore_strndup (abfd, note->descdata + 40, 16);
 	elf_tdata (abfd)->core->command
Index: binutils/bfd/elfn32-mips.c
===================================================================
--- binutils.orig/bfd/elfn32-mips.c	2017-11-08 21:05:36.489887759 +0000
+++ binutils/bfd/elfn32-mips.c	2017-11-08 21:05:37.562322680 +0000
@@ -3562,6 +3562,8 @@ elf32_mips_grok_psinfo (bfd *abfd, Elf_I
 	return FALSE;
 
       case 128:		/* Linux/MIPS elf_prpsinfo */
+	elf_tdata (abfd)->core->pid
+	 = bfd_get_32 (abfd, note->descdata + 16);
 	elf_tdata (abfd)->core->program
 	 = _bfd_elfcore_strndup (abfd, note->descdata + 32, 16);
 	elf_tdata (abfd)->core->command

[committed v7 1/3] BFD: Write Linux core PRSTATUS note into MIPS core file

[Maciej W. Rozycki]

From: Djordje Todorovic <djordje.todorovic@rt-rk.com>

On MIPS o32, n32 and n64 platforms information such as PID was not
correctly written into core file from GDB.

This fixes that behavior.

bfd/ChangeLog:

	* elf32-mips.c (elf32_mips_write_core_note): New function.
	(elf_backend_write_core_note): New macro.
	* elf64-mips.c (elf64_mips_write_core_note): New function.
	(elf_backend_write_core_note): New macro.
	* elfn32-mips.c (elf32_mips_write_core_note): New function.
	(elf_backend_write_core_note): New macro.
---
 bfd/elf32-mips.c  |   44 ++++++++++++++++++++++++++++++++++++++++++++
 bfd/elf64-mips.c  |   44 ++++++++++++++++++++++++++++++++++++++++++++
 bfd/elfn32-mips.c |   44 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 132 insertions(+)

Index: binutils/bfd/elf32-mips.c
===================================================================
--- binutils.orig/bfd/elf32-mips.c	2017-11-07 21:06:16.465919208 +0000
+++ binutils/bfd/elf32-mips.c	2017-11-07 21:06:21.674163289 +0000
@@ -2373,6 +2373,45 @@ elf32_mips_grok_psinfo (bfd *abfd, Elf_I
 
   return TRUE;
 }
+
+/* Write Linux core PRSTATUS note into core file.  */
+
+static char *
+elf32_mips_write_core_note (bfd *abfd, char *buf, int *bufsiz, int note_type,
+			     ...)
+{
+  switch (note_type)
+    {
+    default:
+      return NULL;
+
+    case NT_PRPSINFO:
+      BFD_FAIL ();
+      return NULL;
+
+    case NT_PRSTATUS:
+      {
+	char data[256];
+	va_list ap;
+	long pid;
+	int cursig;
+	const void *greg;
+
+	va_start (ap, note_type);
+	memset (data, 0, 72);
+	pid = va_arg (ap, long);
+	bfd_put_32 (abfd, pid, data + 24);
+	cursig = va_arg (ap, int);
+	bfd_put_16 (abfd, cursig, data + 12);
+	greg = va_arg (ap, const void *);
+	memcpy (data + 72, greg, 180);
+	memset (data + 252, 0, 4);
+	va_end (ap);
+	return elfcore_write_note (abfd, buf, bufsiz,
+				   "CORE", note_type, data, sizeof (data));
+      }
+    }
+}
 \f
 /* Depending on the target vector we generate some version of Irix
    executables or "normal" MIPS ELF ABI executables.  */
@@ -2554,6 +2593,9 @@ static const struct ecoff_debug_swap mip
 #define ELF_COMMONPAGESIZE		0x1000
 #define elf32_bed			elf32_tradbed
 
+#undef elf_backend_write_core_note
+#define elf_backend_write_core_note	elf32_mips_write_core_note
+
 /* Include the target file again for this target.  */
 #include "elf32-target.h"
 
@@ -2575,6 +2617,8 @@ static const struct ecoff_debug_swap mip
 #undef	elf32_bed
 #define elf32_bed				elf32_fbsd_tradbed
 
+#undef elf_backend_write_core_note
+
 #include "elf32-target.h"
 /* Implement elf_backend_final_write_processing for VxWorks.  */
 
Index: binutils/bfd/elf64-mips.c
===================================================================
--- binutils.orig/bfd/elf64-mips.c	2017-11-07 21:06:16.477056388 +0000
+++ binutils/bfd/elf64-mips.c	2017-11-07 21:06:21.680225748 +0000
@@ -4248,6 +4248,45 @@ elf64_mips_grok_psinfo (bfd *abfd, Elf_I
 
   return TRUE;
 }
+
+/* Write Linux core PRSTATUS note into core file.  */
+
+static char *
+elf64_mips_write_core_note (bfd *abfd, char *buf, int *bufsiz, int note_type,
+			     ...)
+{
+  switch (note_type)
+    {
+    default:
+      return NULL;
+
+    case NT_PRPSINFO:
+      BFD_FAIL ();
+      return NULL;
+
+    case NT_PRSTATUS:
+      {
+	char data[480];
+	va_list ap;
+	long pid;
+	int cursig;
+	const void *greg;
+
+	va_start (ap, note_type);
+	memset (data, 0, 112);
+	pid = va_arg (ap, long);
+	bfd_put_32 (abfd, pid, data + 32);
+	cursig = va_arg (ap, int);
+	bfd_put_16 (abfd, cursig, data + 12);
+	greg = va_arg (ap, const void *);
+	memcpy (data + 112, greg, 360);
+	memset (data + 472, 0, 8);
+	va_end (ap);
+	return elfcore_write_note (abfd, buf, bufsiz,
+				   "CORE", note_type, data, sizeof (data));
+      }
+    }
+}
 \f
 /* ECOFF swapping routines.  These are used when dealing with the
    .mdebug section, which is in the ECOFF debugging format.  */
@@ -4454,6 +4493,9 @@ const struct elf_size_info mips_elf64_si
 #define ELF_COMMONPAGESIZE		0x1000
 #define elf64_bed			elf64_tradbed
 
+#undef elf_backend_write_core_note
+#define elf_backend_write_core_note	elf64_mips_write_core_note
+
 /* Include the target file again for this target.  */
 #include "elf64-target.h"
 
@@ -4476,4 +4518,6 @@ const struct elf_size_info mips_elf64_si
 #undef	elf64_bed
 #define elf64_bed				elf64_fbsd_tradbed
 
+#undef elf64_mips_write_core_note
+
 #include "elf64-target.h"
Index: binutils/bfd/elfn32-mips.c
===================================================================
--- binutils.orig/bfd/elfn32-mips.c	2017-11-07 21:06:16.483139578 +0000
+++ binutils/bfd/elfn32-mips.c	2017-11-07 21:06:21.694354823 +0000
@@ -3632,6 +3632,45 @@ elf_n32_mips_grok_freebsd_prstatus (bfd 
   return _bfd_elfcore_make_pseudosection (abfd, ".reg",
 					  size, note->descpos + offset);
 }
+
+/* Write Linux core PRSTATUS note into core file.  */
+
+static char *
+elf32_mips_write_core_note (bfd *abfd, char *buf, int *bufsiz, int note_type,
+			     ...)
+{
+  switch (note_type)
+    {
+    default:
+      return NULL;
+
+    case NT_PRPSINFO:
+      BFD_FAIL ();
+      return NULL;
+
+    case NT_PRSTATUS:
+      {
+	char data[440];
+	va_list ap;
+	long pid;
+	int cursig;
+	const void *greg;
+
+	va_start (ap, note_type);
+	memset (data, 0, 72);
+	pid = va_arg (ap, long);
+	bfd_put_32 (abfd, pid, data + 24);
+	cursig = va_arg (ap, int);
+	bfd_put_16 (abfd, cursig, data + 12);
+	greg = va_arg (ap, const void *);
+	memcpy (data + 72, greg, 360);
+	memset (data + 432, 0, 8);
+	va_end (ap);
+	return elfcore_write_note (abfd, buf, bufsiz,
+				   "CORE", note_type, data, sizeof (data));
+      }
+    }
+}
 \f
 /* Depending on the target vector we generate some version of Irix
    executables or "normal" MIPS ELF ABI executables.  */
@@ -3824,6 +3863,9 @@ static const struct ecoff_debug_swap mip
 #define ELF_COMMONPAGESIZE		0x1000
 #define elf32_bed			elf32_tradbed
 
+#undef elf_backend_write_core_note
+#define elf_backend_write_core_note	elf32_mips_write_core_note
+
 /* Include the target file again for this target.  */
 #include "elf32-target.h"
 
@@ -3846,4 +3888,6 @@ static const struct ecoff_debug_swap mip
 #undef	elf32_bed
 #define elf32_bed				elf32_fbsd_tradbed
 
+#undef elf_backend_write_core_note
+
 #include "elf32-target.h"

[committed v7 3/3] Add test for fetching TLS from core file

[Maciej W. Rozycki]

From: Djordje Todorovic <djordje.todorovic@rt-rk.com>

A correct PID is needed by `libthread_db' library supplied with
glibc repository revisions before commit c579f48edba8 ("Remove
cached PID/TID in clone") or versions before 2.25 release for
GDB to fetch value of TLS variable from core file.  On MIPS
platforms it was omitted and fetching value of TLS variable was not 
available.

This adds a new test in order to be sure if GDB on native platforms
can successfully fetch value of TLS variable.

gdb/testsuite:

	* gdb.threads/tls-core.c: New file.
	* gdb.threads/tls-core.exp: Likewise.
---
 gdb/testsuite/gdb.threads/tls-core.c   |   37 +++++++++++++++++++++
 gdb/testsuite/gdb.threads/tls-core.exp |   56 +++++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+)
 create mode 100644 gdb/testsuite/gdb.threads/tls-core.c
 create mode 100644 gdb/testsuite/gdb.threads/tls-core.exp

Index: binutils/gdb/testsuite/gdb.threads/tls-core.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/gdb/testsuite/gdb.threads/tls-core.c	2017-11-07 21:06:38.057852979 +0000
@@ -0,0 +1,37 @@
+/* This test is part of GDB, the GNU debugger.
+
+   Copyright 2017 Free Software Foundation, Inc.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include <pthread.h>
+
+int __thread foo = 0xdeadbeef;
+
+static void *
+thread_proc (void *arg)
+{
+  return arg;
+}
+
+int
+main (void)
+{
+  pthread_t thread;
+
+  pthread_create (&thread, NULL, thread_proc, NULL);
+  pthread_join (thread, NULL);
+
+  return 0;
+}
Index: binutils/gdb/testsuite/gdb.threads/tls-core.exp
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ binutils/gdb/testsuite/gdb.threads/tls-core.exp	2017-11-07 21:40:30.604166048 +0000
@@ -0,0 +1,56 @@
+# Copyright 2017 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+
+standard_testfile
+
+if {[gdb_compile_pthreads "${srcdir}/${subdir}/${srcfile}" "${binfile}" \
+	executable { debug }] != "" } {
+    return -1
+}
+
+
+clean_restart ${binfile}
+
+runto thread_proc
+
+#
+# Generate corefile.
+#
+set corefile [standard_output_file gcore.test]
+set core_supported [gdb_gcore_cmd "$corefile" "save a corefile"]
+if {!$core_supported} {
+    return 0
+}
+
+#
+# Restart gdb and load generated corefile.
+#
+clean_restart ${binfile}
+
+set core_loaded [gdb_core_cmd "$corefile" "load generated corefile"]
+if { $core_loaded != 1 } {
+    # No use proceeding from here.
+    return 0
+}
+
+# This fails in cross-debugging due to the use of native `libthread_db'.
+if {![string match $host_triplet $target_triplet]} {
+    setup_kfail "threads/22381" "*-*-*"
+}
+gdb_test "p/x foo" \
+	"\\$\[0-9]+ = 0xdeadbeef" \
+	"print thread-local storage variable"
+
+gdb_exit

Re: [PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Djordje Todorovic]

Hi Maciej,

Thanks for committing this, I appreciate it.

Best regards,
Djordje

Re: [PATCH 0/3] Fix issues with writing Linux core PRSTATUS note on MIPS o32, n32 and n64 into core file

[Maciej W. Rozycki]

Hi Djordje,

> Thanks for committing this, I appreciate it.

 You are welcome, it is my duty after all.

  Maciej