Page MenuHomePhabricator

[libunwind][MIPS]: Add support for unwinding in N32 processes.
ClosedPublic

Authored by bsdjhb on Oct 18 2017, 3:02 PM.

Details

Summary

N32 uses the same register context as N64. However, N32 requires one
change to properly fetch addresses from registers stored in memory.
Since N32 is an ILP32 platform, getP() only fetches the first 32-bits
of a stored register. For a big-endian platform this fetches the
upper 32-bits which will be zero. To fix this, add a new
getRegister() method to AddressSpace which is responsible for
extracting the address stored in a register in memory. This matches
getP() for all current ABIs except for N32 where it reads the 64-bit
register and returns the low 32-bits as an address. The
DwarfInstructions::getSavedRegister() method uses
AddressSpace::getRegister() instead of AddressSpace::getP().

Possibly, DwarfInstructions::getSavedRegister()'s return type should
be changed from pint_t to uint64_t.

Event Timeline

bsdjhb created this revision.Oct 18 2017, 3:02 PM
sdardis added inline comments.Oct 24 2017, 4:22 AM
include/__libunwind_config.h
77–86

Change the define of _LIBUNWIND_TARGET_MIPS_N64 to _LIBUNWIND_TARGET_MIPS_NEWABI, then these two elif branches can have the condition (defined(_ABIN32) && defined(_ABI64) and refactored into one elif branch.

80

Shouldn't this 46 be 47?

src/AddressSpace.hpp
233
defined(__mips_n32) -> (defined(__mips__) && defined(_ABIN32))
src/UnwindRegistersRestore.S
859

This line is overly long, break with '\' after the second &&.

src/UnwindRegistersSave.S
176

This line looks overly long, break before the second &&.

src/libunwind.cpp
67–70

Fold these two branches together and have the condition (defined(_ABIN32) || defined(_ABI64), then follow-up by renaming Registers_mips_n64 to Register_mips_NEWABI. Add a comment to the definition of that class stating that it covers both n32 and n64.

bsdjhb updated this revision to Diff 120578.Oct 27 2017, 5:59 AM
bsdjhb marked 3 inline comments as done.
  • Use correct #ifdef for N32.
  • Rename N64 to newabi.
bsdjhb marked 2 inline comments as done.Oct 27 2017, 6:05 AM
bsdjhb added inline comments.
include/__libunwind_config.h
80

No, the other parts of a cursor besides the register context use ILP32 layout for N32 instead of LP64. In particular, I think N32 has less padding after the two 'bool' members at the end of UnwindCursor<>.

bsdjhb updated this revision to Diff 120624.Oct 27 2017, 8:57 AM
  • Rebase after MAX_REGISTER change.
bsdjhb updated this revision to Diff 126620.Dec 12 2017, 1:49 PM
  • Rebase after O32/N64 commit.
compnerd added inline comments.Dec 13 2017, 10:48 AM
include/__libunwind_config.h
79

Minor nit: I prefer either NABI or NEW_ABI.

src/AddressSpace.hpp
233

Can you use __SIZEOF_POINTER__ rather than __LP64__ please? The former accounts for LLP64 environments as well.

src/UnwindRegistersRestore.S
859

clang-format should also fix the width correctly.

sdardis added inline comments.Dec 13 2017, 12:13 PM
include/__libunwind_config.h
79

Normally mips documentation/source code either spells out n32/n64 when referring to the 64 bit abis or calls it a variation of NewABI/newabi/NEWABI (all one word).

src/Registers.hpp
2252–2253 ↗(On Diff #126620)

Nit on the wording:

Registers_mips_newabi holds the register state of a thread in a NEWABI MIPS process including both the N32 and N64 ABIs. ->

Registers_mips_newabi holds the register state of a thread in a MIPS process using NEWABI (the N32 or N64 ABIs).

aprantl removed a subscriber: aprantl.Dec 13 2017, 12:48 PM
bsdjhb marked 2 inline comments as done.Dec 13 2017, 2:13 PM
bsdjhb added inline comments.
src/AddressSpace.hpp
233

I wonder if we should adjust the #if condition used to control the types of pint_t and sint_t earlier in this file to also use SIZEOF_POINTER (as a separate change)?

bsdjhb updated this revision to Diff 126841.Dec 13 2017, 2:15 PM
bsdjhb marked an inline comment as done.
  • Use SIZEOF_POINTER instead of LP64.
  • Adjust comment for newabi register class.

Looking over this patch again, I think I really would prefer that this was split up into two patches. The first one should be entirely mechanical, replacing n64 with newabi. The second patch would actually make the changes that you are are after. That would really help with focusing what the issue here actually is. I don't see anything technically that is an issue (I admit I didn't verify the sizes, but the assertions should hopefully catch that). Beyond that split up, Id like to get a signoff from @sdardis for the MIPS specific bits, but from the libunwind side, LGTM.

Ok, I'm definitely fine with splitting the rename out into a separate patch. Will wait for @sdardis to be sure.

bsdjhb updated this revision to Diff 129103.Jan 9 2018, 9:20 AM
  • Rebase after N64 -> newabi commit.

Sorry for the delay in getting back to this, but testing this by building it explicitly for N32 (I built a full N32 compiler + libunwind and modified the test setup py to compile for N322) and I'm getting crashes on the return path in _Unwind_Backtrace.

The problem occurs when saving the registers, as saving the (second I believe) last register clobbers the saved return address. I've seen crashes on MIPS64 (eb). Qemu however is aborting at runtime, but I haven't had the time to trace the execution, and I haven't found a way yet to dump the locations of the shared libraries at runtime to determine where the fault lies.

This was with GCC 4.9.2 (debian) on MIPS64 and our 2016.05-06 toolchain for qemu.

To be clear, are you getting the failure running libunwind's test suite or your own test? I've managed to get libunwind to cross-compile for me using GCC 6.3.0 on FreeBSD for O32, N32, and N64, but only to build the library, not the tests. I've been running a simple C++ test program (which is using a patched libunwind along with libc++ as it's C++ runtime) for testing. The program uses _Unwind_Backtrace() as well as throws a couple of C++ exceptions with catch handlers that print out the values thrown. If you are able to cross-build the libunwind tests and then run them under qemu I'd appreciate a pointer to get that working as I'd be happier running libunwind's tests than my own.

This was libunwind's test suite:

Compiled test failed unexpectedly!
********************
Testing Time: 0.53s
********************
Failing Tests (1):
    libunwind :: libunwind_02.pass.cpp

  Expected Passes    : 3
  Unexpected Failures: 1

The hacky patch I used to test n32:

--- a/test/libunwind/test/config.py
+++ b/test/libunwind/test/config.py
@@ -48,6 +48,8 @@ class Configuration(LibcxxConfiguration):
         # Stack unwinding tests need unwinding tables and these are not
         # generated by default on all Targets.
         self.cxx.compile_flags += ['-funwind-tables']
+        self.cxx.compile_flags += ['-mabi=n33']
+        self.cxx.link_flags += ['-mabi=n32']
         if not self.get_lit_bool('enable_threads', True):
             self.cxx.compile_flags += ['-D_LIBUNWIND_HAS_NO_THREADS']
             self.config.available_features.add('libunwind-no-threads')
sdardis added a comment.EditedJan 12 2018, 11:46 AM

Um, I now appear to be getting different results for running under QEMU doing it the proper way. I was previously rebuilding the failing test by hand and running under qemu. I don't believe I changed anything important, I'll have to take a longer look.

If you define an Executor for libunwind you can run the testsuite under QEMU, automagically.

I have:

LIBUNWIND_EXECUTOR               PrefixExecutor(['/home/sdardis/bin/qemu-mipsn32.sh'],LocalExecutor())

/home/sdardis/bin/qemu-mips32.sh for is a simple bash script:

#!/bin/bash
~/mips-mti-linux-gnu/2016.05-06/bin/qemu-mipsn32 -L /home/sdardis/mips-mti-linux-gnu/2016.05-06/sysroot/mips-r2-hard/ -E LD_LIBRARY_PATH=/home/sdardis/mips-mti-linux-gnu/2016.05-06/mips-mti-linux-gnu/lib/mips-r2-hard/lib32/ "$@"

Hope this helps.

Thanks for all the work you're putting into this.

This was libunwind's test suite:

Compiled test failed unexpectedly!
********************
Testing Time: 0.53s
********************
Failing Tests (1):
    libunwind :: libunwind_02.pass.cpp

  Expected Passes    : 3
  Unexpected Failures: 1

The hacky patch I used to test n32:

--- a/test/libunwind/test/config.py
+++ b/test/libunwind/test/config.py
@@ -48,6 +48,8 @@ class Configuration(LibcxxConfiguration):
         # Stack unwinding tests need unwinding tables and these are not
         # generated by default on all Targets.
         self.cxx.compile_flags += ['-funwind-tables']
+        self.cxx.compile_flags += ['-mabi=n33']
+        self.cxx.link_flags += ['-mabi=n32']
         if not self.get_lit_bool('enable_threads', True):
             self.cxx.compile_flags += ['-D_LIBUNWIND_HAS_NO_THREADS']
             self.config.available_features.add('libunwind-no-threads')

Just to be sure, is that '-mabi=n33' in the compile flags a copy and paste typo in the diff or do you have it locally in the real change as well?

I just checked both my qemu copy and on my mips64 machine and it seems to be a a copy / paste error. Reposting here directly from my machines:

MIPS64:

diff --git a/test/libunwind/test/config.py b/test/libunwind/test/config.py
index 2a0c828..a8952c3 100644
--- a/test/libunwind/test/config.py
+++ b/test/libunwind/test/config.py
@@ -48,6 +48,8 @@ class Configuration(LibcxxConfiguration):
         # Stack unwinding tests need unwinding tables and these are not
         # generated by default on all Targets.
         self.cxx.compile_flags += ['-funwind-tables']
+       self.cxx.compile_flags += ['-mabi=n32']
+        self.cxx.link_flags += ['-mabi=n32']
         if not self.get_lit_bool('enable_threads', True):
             self.cxx.compile_flags += ['-D_LIBUNWIND_HAS_NO_THREADS']
             self.config.available_features.add('libunwind-no-threads')

X86_64:

diff --git a/test/libunwind/test/config.py b/test/libunwind/test/config.py
index 2a0c828..f1953e2 100644
--- a/test/libunwind/test/config.py
+++ b/test/libunwind/test/config.py
@@ -48,6 +48,8 @@ class Configuration(LibcxxConfiguration):
         # Stack unwinding tests need unwinding tables and these are not
         # generated by default on all Targets.
         self.cxx.compile_flags += ['-funwind-tables']
+       self.cxx.compile_flags += ['-mabi=n32']
+        self.cxx.link_flags += ['-mabi=n32', '/home/sdardis/mips-mti-linux-gnu/2016.05-06/bin/../lib/gcc/mips-mti-linux-gnu/4.9.2/../../../../mips-mti-linux-gnu/lib/mips-r2-hard/lib32/libgcc_s.so.1']
         if not self.get_lit_bool('enable_threads', True):
             self.cxx.compile_flags += ['-D_LIBUNWIND_HAS_NO_THREADS']
             self.config.available_features.add('libunwind-no-threads')

I corrected whitespace nit before I posted the comment.

After fighting with cmake for a bit, I just broke down and cross-compiled the tests by hand and then ran them under a qemu system (rather than using qemu user mode). All of the tests ran fine for me without crashing using GCC 6.3.0 for FreeBSD 12 with N32. Given the save/restore code is identical for N32 and N64 I would have expected N64 to fail previously if it was restoring the wrong register?

bsdjhb added a comment.Feb 2 2018, 3:05 PM

@sdardis Can you confirm if the existing N64 bits work fine for you or if the tests crash similarly?

Ok, I've found my issue. inttypes.h in libcxx include_next's <inttypes.h>, which on my debian linux systems has the include chain <features.h>, <stubs.h>, <sgidefs.h>. <sgidefs.h> "helpfully" provides #defines of the various _ABIXXX macros, which normally the compiler defines depending on the ABI. The include chain for other parts of libunwind differ which means src/UnwindLevel1-gcc-ext.c had a different definition of the stuct unw_context_t, which was the version defined for O32. As GCC and clang laid out the stack differently for each ABI differently the bug was masked. However for N32 at O3, the layout was such that the unwind context in the locals area was right below the saved registers for _Unwind_Backtrace which trashed older saved values, then triggered a SIGSEGV on return due to reloading the saved contents of the $lo into $ra in my case.

The fix is fairly simple. Change the ABI #ifdef defined() checks to be:

#ifdef _defined(_ABIO32) && _MIPS_SIM == _ABIO32

for O32 or similar for a single ABI. For N32 and N64 together, it's sufficient to test for the presence of __mips64.

include/__libunwind_config.h
74

See my comment about the N32 abi check.

78

This needs to be:

#    elif defined(_ABIN32) && _MIPS_SIM == _ABIN32
78–82

Same here.

src/AddressSpace.hpp
233

Change that mips check to defined(__mips64) which covers the target being mips and it being n64 or n32.

src/UnwindRegistersRestore.S
859

Rather checking that the _ABI64 or _ABIN32 are defined, check that __mips64 is defined.

src/UnwindRegistersSave.S
176

See my comment on register restore file.

src/libunwind.cpp
67

Check that __mips64 is defined rather than the specific _ABI macros.

bsdjhb updated this revision to Diff 133660.Feb 9 2018, 11:11 AM
  • Rebase.
  • Rework ABI macro checks.
bsdjhb marked 7 inline comments as done.Feb 9 2018, 11:13 AM

Nice sleuthing!

The only thing this needs now is to get correct testing support. Could you add support for passing down to the configuration script an additional set of cflags like compiler-rt and libomp do (as a separate patch)? If you look at libomp, you'll see LIBOMP_TEST_CFLAGS defined in the cmake build system and threaded through various places so the built tests can have the correct options. Without supporting that we get a mixed set of objects, which isn't correct.

Hmmm, I'm a bit lost on the CFLAGS bit. I couldn't find a reference to LIBOMP_TEST_CFLAGS anywhere in the openmp tree. There is a LIBOMP_CFLAGS that doesn't appear to be test specific.

To try to find a way to modify the CFLAGS for tests I looked at how LIBUNWIND_BUILD_32_BITS works. It sets a value (config.enable_32bit) in test/lit.site.cfg.in. This is then used by the utils/libcxx/test/config.py script in the libcxx repository in the configure_default_compile_flags() function to add -m32 to CFLAGS. There isn't an existing config.* knob that can add arbitrary things to CFLAGS though, so I think adding a LIBUNWIND_TEST_CFLAGS would mean changing the config.py to learn about a new 'config.cflags' or the like and then mapping LIBUNWIND_TEST_CFLAGS to that via lit.site.cfg.in?

Yes. Something like LIBUNWIND_TEST_CFLAGS mapping to config.test_cflags, which libunwind/test/libunwind/test/config.py then appends when building the tests.

Thanks.

sdardis accepted this revision.Feb 26 2018, 7:51 AM

LGTM after D43585 lands.

This revision is now accepted and ready to land.Feb 26 2018, 7:51 AM
This revision was automatically updated to reflect the committed changes.