public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
From: Kevin Buettner <kevinb@redhat.com>
To: gdb-patches@sourceware.org
Subject: [PATCH v2 2/5] Adjust corefile.exp test to show regression after bfd hack removal
Date: Wed, 13 May 2020 10:11:52 -0700	[thread overview]
Message-ID: <20200513171155.645761-3-kevinb@redhat.com> (raw)
In-Reply-To: <20200513171155.645761-1-kevinb@redhat.com>

In his review of my BZ 25631 patch series, Pedro was unable to
reproduce the regression which should occur after patch #1, "Remove
hack for GDB which sets the section size to 0", is applied.

Pedro was using an ld version older than 2.30.  Version 2.30
introduced the linker option -z separate-code.  Here's what the man
page has to say about it:

    Create separate code "PT_LOAD" segment header in the object.  This
    specifies a memory segment that should contain only instructions
    and must be in wholly disjoint pages from any other data.

In ld version 2.31, use of separate-code became the default for
Linux/x86.  So, really, 2.31 or later is required in order to see the
regression that occurs in recent Linux distributions when only patch

For the test case in question, use of the separate-code linker option
means that the global variable "coremaker_ro" ends up in a separate
load segment (though potentially with other read-only data).  The
upshot of this is that when only patch #1 is applied, GDB won't be
able to correctly access coremaker_ro.  The reason for this is due
to the fact that this section will now have a non-zero size, but
will not have an contents from the core file to find this data.
So GDB will ask BFD for the contents and BFD will respond with
zeroes for anything from those sections.  GDB should instead be
looking in the executable for this data.  Failing that, it can
then ask BFD for a reasonable value.  This is what a later patch
in this series does.

When using ld versions earlier than 2.31 (or 2.30 w/ the
-z separate-code option explicitly provided to the linker), there is
the possibility that coremaker_ro ends up being placed near other data
which is recorded in the core file.  That means that the correct value
will end up in the core file, simply because it resides on a page that
the kernel chooses to put in the core file.  This is why Pedro wasn't
able to reproduce the regression that should occur after fixing the
BFD hack.

This patch places a big chunk of memory, two pages worth on x86, in
front of "coremaker_ro" to attempt to force it onto another page
without requiring use of that new-fangled linker switch.

Speaking of which, I considered changing the test to use
-z separate-code, but this won't work because it didn't
exist prior to version 2.30.  The linker would probably complain
of an unrecognized switch.  Also, it likely won't be available in
other linkers not based on current binutils.  I.e.  it probably won't
work in FreeBSD, NetBSD, etc.

To make this more concrete, this is what *should* happen when
attempting to access coremaker_ro when only patch #1 is applied:

    Core was generated by `/mesquite2/sourceware-git/f28-coresegs/bld/gdb/testsuite/outputs/gdb.base/coref'.
    Program terminated with signal SIGABRT, Aborted.
    #0  0x00007f68205deefb in raise () from /lib64/libc.so.6
    (gdb) p coremaker_ro
    $1 = 0

Note that this result is wrong; 201 should have been printed instead.
But that's the point of the rest of the patch series.

However, without this commit, or when using an old Linux distro with
a pre-2.31 ld, this is what you might see instead:

    Core was generated by `/mesquite2/sourceware-git/f28-coresegs/bld/gdb/testsuite/outputs/gdb.base/coref'.
    Program terminated with signal SIGABRT, Aborted.
    #0  0x00007f63dd658efb in raise () from /lib64/libc.so.6
    (gdb) p coremaker_ro
    $1 = 201

I.e. it prints the right answer, which sort of makes it seem like the
rest of the series isn't required.

Now, back to the patch itself... what should be the size of the memory
chunk placed before coremaker_ro?

It needs to be at least as big as the page size (PAGE_SIZE) from
the kernel.  For x86 and several other architectures this value is
4096.  I used MAPSIZE which is defined to be 8192 in coremaker.c.
So it's twice as big as what's currently needed for most Linux
architectures.  The constant PAGE_SIZE is available from <sys/user.h>,
but this isn't portable either.  In the end, it seemed simpler to
just pick a value and hope that it's big enough.  (Running a separate
program which finds the page size via sysconf(_SC_PAGESIZE) and then
passes it to the compilation via a -D switch seemed like overkill
for a case which is rendered moot by recent linker versions.)

Further information can be found here:

   https://sourceware.org/pipermail/gdb-patches/2020-May/168168.html
   https://sourceware.org/pipermail/gdb-patches/2020-May/168170.html

Thanks to H.J. Lu for telling me about the '-z separate-code' linker
switch.

gdb/testsuite/ChangeLog:

	* gdb.base/coremaker.c (filler_ro): New global constant.
---
 gdb/testsuite/gdb.base/coremaker.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/gdb/testsuite/gdb.base/coremaker.c b/gdb/testsuite/gdb.base/coremaker.c
index 55330fd3e8..4c41b9c926 100644
--- a/gdb/testsuite/gdb.base/coremaker.c
+++ b/gdb/testsuite/gdb.base/coremaker.c
@@ -42,6 +42,12 @@ char *buf2;
 int coremaker_data = 1;	/* In Data section */
 int coremaker_bss;	/* In BSS section */
 
+/* Place a chunk of memory before coremaker_ro to improve the chances
+   that coremaker_ro will end up on it's own page.  See:
+
+   https://sourceware.org/pipermail/gdb-patches/2020-May/168168.html
+   https://sourceware.org/pipermail/gdb-patches/2020-May/168170.html  */
+const unsigned char filler_ro[MAPSIZE] = {1, 2, 3, 4, 5, 6, 7, 8};
 const int coremaker_ro = 201;	/* In Read-Only Data section */
 
 /* Note that if the mapping fails for any reason, we set buf2
-- 
2.25.4


  parent reply	other threads:[~2020-05-13 17:13 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-13 17:11 [PATCH v2 0/5] Fix BZ 25631 - core file memory access problem Kevin Buettner
2020-05-13 17:11 ` [PATCH v2 1/5] Remove hack for GDB which sets the section size to 0 Kevin Buettner
2020-05-13 17:11 ` Kevin Buettner [this message]
2020-05-20 16:24   ` [PATCH v2 2/5] Adjust corefile.exp test to show regression after bfd hack removal Pedro Alves
2020-05-13 17:11 ` [PATCH v2 3/5] section_table_xfer_memory: Replace section name with callback predicate Kevin Buettner
2020-05-20 16:33   ` Pedro Alves
2020-05-13 17:11 ` [PATCH v2 4/5] Provide access to non SEC_HAS_CONTENTS core file sections Kevin Buettner
2020-05-20 16:45   ` Pedro Alves
2020-05-21  7:50     ` Kevin Buettner
2020-05-21 12:40       ` Pedro Alves
2020-05-21 14:23         ` Pedro Alves
2020-05-21 15:09           ` Kevin Buettner
2020-05-21 16:28             ` Pedro Alves
2020-05-21 17:06       ` Pedro Alves
2020-05-13 17:11 ` [PATCH v2 5/5] Test ability to access unwritten-to mmap data in core file Kevin Buettner
2020-05-20 16:46   ` Pedro Alves

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200513171155.645761-3-kevinb@redhat.com \
    --to=kevinb@redhat.com \
    --cc=gdb-patches@sourceware.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).