This is an archive of the discontinued LLVM Phabricator instance.

[libunwind][AIX] implementation of the unwinder for AIX
ClosedPublic

Authored by xingxue on Apr 8 2021, 11:48 AM.

Details

Summary

This patch contains the implementation of the unwinder for IBM AIX.

  • AIX does not support the eh_frame section. Instead, the traceback table located at the end each function provides the information for stack unwinding and EH. In this patch macro _LIBUNWIND_SUPPORT_TBTAB_UNWIND is used to guard code for AIX traceback table based unwinding. Function getInfoFromTBTable() and stepWithTBTable() are added to get the EH information from the traceback table and to step up the stack respectively.
  • This patch supports two kinds of LSDA information for EH on AIX, the state table and the range table. The state table is used by the previous version of the IBM XL compiler, i.e., xlC and xlclang++. The DWARF based range table is used by AIX clang++. The traceback table has flags to differentiate these cases. For the range table, relative addresses are calculated using a base of DW_EH_PE_datarel, which is the TOC base of the module where the function of the current frame belongs.
  • Two personality routines are employed to handle these two different LSDAs, __xlcxx_personality_v0() for the state table and __xlcxx_personality_v1() for the range table. Since the traceback table does not have the information of the personality for the state table approach, its personality __xlcxx_personality_v0() is dynamically resolved as handler for the state table. For the range table, the locations of the LSDA and its associated personality routine are found in the traceback table.
  • Assembly code for 32- and 64-bit PowerPC in UnwindRegistersRestore.S and UnwindRegistersSave.S are modified so that it can be consumed by the GNU flavor assembler and the AIX assembler. The restoration of vector registers does not check VRSAVE because in the AIX ABI, VRSAVE is not used.
  • Changes in libcxxabi for the AIX EH is in a separate patch D100504.
  • No issues were found running libunwind and libcxxabi LIT tests. LIT test cases have been added for code specific to AIX unwinding.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Renamed variables in Unwind_AIXExtras.cpp.

xingxue updated this revision to Diff 336568.Apr 9 2021, 2:50 PM

Fixed the issue caused by git-clang-format in file assembly.h.

ldionne resigned from this revision.Apr 12 2021, 11:45 AM
ldionne added a subscriber: steven_wu.

Dear Reviewers.

The pre-commit check of this patch flagged clang-format and clang-tidy problems.

  • The clang-tidy failures are related to using of undeclared identifier UNW_STEP_END and UNW_STEP_SUCCESS. These macros are defined in <libunwind_ext.h> without any guarding conditions. I was able to build the patch on a PowerPC Linux system running 'Ubuntu 18.04.5' as well as AIX successfully. Maybe I missed something but I was unable to figure out why the AIX unwinder code would affect UNW_STEP_END and UNW_STEP_SUCCESS. It would be greatly appreciated if you could provide guidance on resolving it.
  • The clang-format flags the following issues <assembly.h>. I cannot really put a space between % and r as suggested because this represent the registers in assembly language, e.g., %r1, %f1. The assembler will complain if it is % r1, % f1. Again, any help would be greatly appreciated.
clang-format: please reformat the code

-#define GPR(num) %r##num
-#define FPR(num) %f##num
-#define VSR(num) %vs##num
-#define VR(num) %v##num
+#define GPR(num) % r##num
+#define FPR(num) % f##num
+#define VSR(num) % vs##num
+#define VR(num) % v##num
xingxue updated this revision to Diff 337240.Apr 13 2021, 12:51 PM

Fixed some more assembly issues from the formatter.

lkail added a subscriber: lkail.Apr 14 2021, 12:38 AM
xingxue edited the summary of this revision. (Show Details)Apr 14 2021, 12:54 PM
xingxue added a reviewer: sfertile. xingxue removed 1 blocking reviewer(s): Restricted Project.
Herald added 1 blocking reviewer(s): Restricted Project. · View Herald TranscriptApr 14 2021, 12:54 PM
sfertile added inline comments.Apr 22 2021, 1:30 PM
libunwind/src/UnwindRegistersRestore.S
146

We need to split out the NFC part of the macro changes and commit that in an initial patch to make sure nothing regresses.

xingxue updated this revision to Diff 340133.Apr 23 2021, 12:20 PM
xingxue added a reviewer: steven_wu.

Addressed comment:

xingxue marked an inline comment as done.Apr 23 2021, 12:25 PM
xingxue updated this revision to Diff 343029.May 5 2021, 7:01 AM
xingxue edited the summary of this revision. (Show Details)
xingxue added a reviewer: MaskRay.

Rebased after changes for PowerPC assembly code in NFC patch (https://reviews.llvm.org/D101179).

xingxue updated this revision to Diff 343060.May 5 2021, 8:43 AM

Changes from clang-format.

xingxue updated this revision to Diff 345990.May 17 2021, 2:07 PM

Changes:

  • Dynamically resolve symbol __xlcxx_personality_v0 that is the personality for the state table so that libunwind does not have hard dependency on libc++abi on AIX.
sfertile added inline comments.Jun 11 2021, 8:16 AM
libunwind/src/AddressSpace.hpp
599

minor nit: whitespace change.

libunwind/src/UnwindCursor.hpp
2051

minot nit: I think its cleaner to pull this out of the if block and increment past the name whether we need to or not.

2092

Should failing to load the library, or failing to resolve __xlcxx_personality_v0 be a fatal failure?

2136

This is the EH implementation for both new XLC++ and clang so we should drop the XLC++ from the assert message.

2489

Do we need this on AIX? I think we have 2 cases that could happen when we throw as the last instruction in a function and I don't think we need to backtrack the instruction pointer in either. The first case is a normal call to __cxa_throw, the call instruction will be followed by a toc-restore which is what the program counter will be pointing at. The second case is a call to _cxa_throw with no toc-restore, which might come up if we _cxa_throw is statically linked in and we use LTO or the call is written in asm, but in this case the program counter will be pointing to the word after the call instruction which is the zero word we are looking for (so no need to backtrack).

sfertile added inline comments.Jun 14 2021, 8:52 AM
libunwind/src/Unwind_AIXExtras.cpp
32

Switch the order here and early return if the name is not present.

ie

if (!TBTable->tb.name_present)
  return NULL

// Get the name of the function.
...
sfertile added inline comments.Jun 15 2021, 7:49 AM
libunwind/src/assembly.h
70–71

Should we update the guards so we don't define these macros on AIX?

216

Minor nit: _WEAK_ALIAS to match the other weak alias macros.

217

Same comment as below about this .toc pseudo op.

xingxue updated this revision to Diff 352128.Jun 15 2021, 7:54 AM
xingxue retitled this revision from [libunwind][AIX] Initial patch of the unwinder on AIX to [libunwind][AIX] implementation of the unwinder for AIX.
xingxue edited the summary of this revision. (Show Details)
xingxue edited reviewers, added: compnerd; removed: ldionne.

Addressed comments.

File UnwindCursor.hpp:

  • Pulled the checking and skipping name_len and name fields out of the body of the if statement that checks uses_alloca field for better readability.
  • Added asserts if dlopen() or dlsym() fail.
  • Changed text in assert messages from XLC++ to libunwind.

File Unwind_AIXExtras.cpp:

  • In function getFuncName() switched order to return early.
xingxue marked 6 inline comments as done.Jun 15 2021, 8:24 AM
xingxue added inline comments.
libunwind/src/AddressSpace.hpp
599

There is an extra empty line here and the change is from clang-format. I am leaning towards to keeping the change.

libunwind/src/UnwindCursor.hpp
2051

Changed as suggested, thanks!

2092

The dynamic resolving of symbol __xlcxx_personality_v0 is used for stack frames from code generated by the legacy compiler. According to the compiler packaging, they should not fail. In a unlikely event they do fail, the unwinder will ignore the legacy frames and it is still able to process CLANG generated frames. The debugging trace has the info if it happened. Added asserts to help debug.

2136

Good point! Replaced XLC++ with libunwind.

2489

PC is needed for checking if the location is within a range of addresses.

libunwind/src/Unwind_AIXExtras.cpp
32

Changed as suggested, thanks!

sfertile added inline comments.Jun 15 2021, 10:55 AM
libunwind/src/assembly.h
247

Sorry, I seem to have deleted this comment before submitting my last set of comments. Its extra emabahrasing because one of the comments I left referred to this one :(.

The .toc pseudo is only needed once per file that includes this header, not really once per function being defined (although the extras are harmless). We have a change at the top of the 2 files that include assembly.h to not emit the .text directive, we can instead change that to emit the .toc directive on AIX and delete the .toc directive from this macro and DEFINE_LIBUNWIND_FUNCTION_AND_WEAKALIAS

xingxue updated this revision to Diff 352241.Jun 15 2021, 2:39 PM
xingxue marked 6 inline comments as done.

Addressed comments:

  • Guarded out powerpc64 macros not needed by AIX.
  • Renamed DEFINE_LIBUNWIND_FUNCTION_AND_WEAKALIAS to DEFINE_LIBUNWIND_FUNCTION_AND_WEAK_ALIAS.
  • Removed .toc pseudo op from macros definitions in <assembly.h> and specified it in assembly files instead.
  • Enabled the prototype of _Unwind_GetDataRelBase in <unwind.h> for AIX.
xingxue marked 4 inline comments as done.Jun 15 2021, 2:49 PM
xingxue added inline comments.
libunwind/src/assembly.h
70–71

Changed as suggested, thanks!

216

Renamed the macro as suggested.

217

Good idea, thanks!

247

No worries. Thanks for the good suggestion!

xingxue updated this revision to Diff 352478.Jun 16 2021, 10:45 AM
xingxue marked 4 inline comments as done.

Rebased on the latest source.

xingxue updated this revision to Diff 353964.Jun 23 2021, 7:34 AM

Minor changes.

  • Fixed stack adjustment for restoring vector registers.
  • Fixed a typo in Unwind_AIXExtras.cpp.
xingxue updated this revision to Diff 358260.Jul 13 2021, 7:33 AM

Updated with the following changes.

  • Use the bit ordering of traceback table bits has_vec and longtbtable defined in the AIX OS header <debug.h>.
  • Mitigate the problem when the unwinder hits a frame with the traceback table incorrectly generated by an XL compiler that uses the wrong ordering.
xingxue updated this revision to Diff 383882.Nov 1 2021, 4:09 PM
xingxue edited the summary of this revision. (Show Details)

Updated with the following change.

  • Fixed the weak symbol definition in assembly.h.
xingxue updated this revision to Diff 384095.Nov 2 2021, 7:24 AM

Removed an empty line in UnwindLevel1-gcc-ext.c.

I haven't completed my review. In particular I need to look at the longtbtable, has_vec mix up with more scrutiny but I wanted to submit my comments thus far.

libunwind/include/unwind.h
164

This declaration of _Unwind_GetDataRelBase is the same as the existing one. Isn't this identical to the cleaner:

extern uintptr_t _Unwind_GetDataRelBase(struct _Unwind_Context *context)
    LIBUNWIND_UNAVAIL;
#if !defined(_AIX)
extern uintptr_t _Unwind_GetTextRelBase(struct _Unwind_Context *context)
    LIBUNWIND_UNAVAIL;
#endif
libunwind/src/Registers.hpp
612

formatting is not consistent with surrounding code.

1180

formatting is not consistent with surrounding code.

libunwind/src/UnwindCursor.hpp
1965

I think "oldFrame" could be more descriptive. The term is meant to encompass xlC and xlclang++ compiled EH aware functions.

Suggest "legacyXLFrame" or "classicXLFrame". I think a comment in libunwind wrt limitations of interaction with such frames is warranted, because those frames contain calls to the legacy libC system unwinder, which has potential to foul up state with libunwind.

1973

Don't you want to use the xlcxx_personality_v0_t typedef you just defined to declare xlcxx_personality_v0?

2035

suggestion to your discretion: there are so many offsets in this code I would be inclined to name stateOffset => stateFrameOffset

2040

Curiously the IBM legacy XL code only scans past the name if the traceback table uses_alloca flag is set.

What you've written makes intuitive sense but I'd like to confirm the semantic here is intended.

2062

I think a commented is warranted here that you're trying to prevent libunwind from having a dependency on libc++abi. If a legacy XLC++ frame is encountered, then the loading of libc++abi is warranted.

libunwind/src/assembly.h
71

What is defining _CALL_ELF that it's necessary to mask this off with ~defined(_AIX) ?

xingxue added inline comments.Jan 6 2022, 10:59 AM
libunwind/include/unwind.h
164

I agree, thanks!

xingxue added inline comments.Jan 6 2022, 10:59 AM
libunwind/src/Registers.hpp
612

This is from the git-clang-format.

1180

This is from git-clang-format.

libunwind/src/UnwindCursor.hpp
1965

Replaced oldFrame with classicXLFrame as suggested, thanks!

There should not be limitations of the interaction between the classicXLFrame and the clangFrame in libunwind. The xlclang++ compiled EH using oldFrame does not call the legacy libC unwinder and is compatible with clang compiled EH when running against libunwind/libc++abi. I agree that the legacy xlC compiled EH uses the libC unwinder which is incompatible with libunwind but that is because of the difference in the design of unwinders not the stack frame.

1973

Changed as suggested. Thanks for pointing this out.

2035

Replaced stateOffset with stateTableOffset.

2040

The semantic is the same as the legacy XL code: 1) if uses_alloca flag is set, use the content of the location after the name as the frame pointer register; 2) otherwise, use the default frame pointer register, i.e., SP. Computing the location after the name is unnecessary in the case where uses_alloca is unset. But Sean thought it is cleaner this way (see the comment below).

2062

Added comments explaining why dynamically resolving.

libunwind/src/assembly.h
71

&& !defined(_AIX) is added to mask off the code for AIX, no?

xingxue updated this revision to Diff 397944.Jan 6 2022, 11:26 AM
xingxue removed a reviewer: jasonliu.

Addressed comments:

  • Make the declaration of _Unwind_GetDataRelBase cleaner in unwind.h.
  • Rename enum member oldFrame to classicXLFrame.
  • Use typedef of xlcxx_personality_v0_t in the declaration of __xlcxx_personality_v0().
  • Rename variable stateOffset to stateTableOffset.
  • Add commet to explain why dynamically resolving the personality for the state table.
xingxue marked 9 inline comments as done.Jan 6 2022, 11:29 AM
cebowleratibm added inline comments.Jan 6 2022, 4:18 PM
libunwind/src/UnwindCursor.hpp
2098

Why is the logic for skipping the name here not the same as above? The handling of XL frames bumps the pointer by the name_len.

2106

I don't think this comment is necessary because this branch is for non XL tbtables, which should not mix up the has_vec and longtbtable bits.

2117

I think it's unnecessary to check the reservedBit here because this code should only be handling clang frames at this point that don't mix up the vec_ext and longtbtable bits.

The details of the has_vec and longtbtable bit mix up by the XL compiler can then be pushed into the v0 XL personality routine where the complexity belongs. I don't think the check or comments are necessary here.

2342

Just for my understanding, is this the first instruction of the glink forwarding stub that the linker inserts for toc save/restore?

xingxue added inline comments.Jan 7 2022, 10:27 AM
libunwind/src/UnwindCursor.hpp
2098

Changed to use the same logic to skip the name_len and name fields.

2106

It is possible to enter this branch for a classic XL frame as well. The possible scenarios of entering this branch are: 1) tb.lang == TB_CPLUSPLUS but TBTable->tb.has_ctl is not set, which means it is either a classic XL frame from xlc++/xlclang++ but not EH aware, or a Clang frame. 2) tb.lang != TB_CPLUSPLUS, then it is a frame of other languages, such as C, which can be generated by xlc, xlclang, or clang.

2117

I think it's unnecessary to check the reservedBit here because this code should only be handling clang frames at this point that don't mix up the vec_ext and longtbtable bits.

This code needs to handle classic XL frames as well. See the comment above.

2342

Yes, the compiler generates a 'nop' after a call and the linker replaces it with the instruction to load the TOC saved by the glink stub.

xingxue updated this revision to Diff 398189.Jan 7 2022, 10:39 AM

Addressed comment:

  • Use the same logic for skipping the name_len and name fields.
xingxue marked 4 inline comments as done.Jan 7 2022, 10:42 AM
cebowleratibm added inline comments.Jan 7 2022, 11:26 AM
libunwind/include/unwind.h
164

What is the reason for declaring _Unwind_GetDataRelBase on all platforms but hiding _Unwind_GetTextRelBase on AIX?

libunwind/src/AddressSpace.hpp
50

getFuncName doesn't sound AIX specific so it's odd to see it guarded by _AIX. Perhaps a more specific name to describe why it's unique to AIX?

libunwind/src/UnwindCursor.hpp
1965

clangFrame probably isn't an appropriate name either as we shouldn't imply that libunwind will only work with clang frames. Although clang is the only current emitter of such frames, other compilers may follow suit.

2097

I suggest a comment to explicitly state that we're no handling either a "new" frame or a classic XL frame that is not eh-aware. I think it also good to outline the mix up of the has_vec and longtbtable bits at this point. The reader should understand that there's a case where the longtbtable is set but there isn't really a xtbtable and the code that follows deals with this complexity.

2119

I think it's useful to spell out the erroneous xlC frame case. What situation exposes it and in that case what does reservedBit actually refer to?

Here's my understanding:
-xlC compiled non eh aware frame which has vector registers to restore but nothing that otherwise requires the longtbtable bit to be set (otherwise both bits are set and the mix up is moot.) In this case, the current charPtr will actually be pointing into the vec_ext struct of the tbtable because we failed to scan past the vec_ext struct because the has_vec bit was erroneously false.

As LLVM devs don't have access to internal XL source/docs I feel it's necessary we go above and beyond to explain this clearly in the comments. Even for me it took some time to reason through the purpose of the reservedBit check.

2145

Suggest moving this to the top of the block because it helps the reader identify that we've deduced that frame must be a non classic XL frame.

xingxue marked an inline comment as done.Jan 11 2022, 9:08 AM
xingxue added inline comments.
libunwind/include/unwind.h
164

Good question. We can allow the declaration of _Unwind_GetTextRelBase() on AIX, and have the stub routine print "_Unwind_GetTextRelBase() not implemented" and abort like others.

libunwind/src/AddressSpace.hpp
50

Renamed getFuncName to getFuncNameFromTBTable.

libunwind/src/UnwindCursor.hpp
1965

Renamed clangFrame to frameWithEHInfo and renamed classicFrame to frameWithXLEHStateTable.

2097

Added comments as suggested. Thanks!

2119

Added comments at the beginning of the branch to describe the erroneous xlC frame case. Updated the comment here accordingly.

2145

Moved the statement to the top of the block as suggested, thanks!

xingxue updated this revision to Diff 398978.Jan 11 2022, 9:27 AM
xingxue marked an inline comment as done.

Addressed comments.

  • allow the declaration of _Unwind_GetTextRelBase() on AIX.
  • rename getFuncName to getFuncNameFromTBTable.
  • rename clangFrame to frameWithEHInfo and rename classicFrame to frameWithXLEHStateTable.
  • add comments to describe the code looking for eh_info and dealing with the complexity arising from some XL compiler versions use the wrong ordering of flag bits.
xingxue marked 5 inline comments as done.Jan 11 2022, 9:33 AM
cebowleratibm accepted this revision.Jan 12 2022, 10:00 AM

My remaining suggestions are very minor. I've had a thorough look through this patch though I'll look to others to approve the patch to be committed.

I'm confident the functionality here is sound as it is representative of the libunwind implementation deployed on AIX alongside the Open XL V17 compiler.

libunwind/src/AddressSpace.hpp
48
630
libunwind/src/UnwindCursor.hpp
2104
2113
libunwind/src/Unwind_AIXExtras.cpp
21

I suggested guarding this function with _LIBUNWIND_SUPPORT_TBTAB_UNWIND so perhaps an asserting stub is appropriate on non-AIX targets.

xingxue marked 5 inline comments as done.Jan 12 2022, 12:34 PM
xingxue added inline comments.
libunwind/src/AddressSpace.hpp
48

It is guarded by defined(_AIX) instead of defined(_LIBUNWIND_SUPPORT_TBTAB_UNWIND) because getFuncNameFromTBTable() would also be used for GCC on AIX but GCC does not use the traceback table for unwinding.

630

It is guarded by defined(_AIX) instead of defined(_LIBUNWIND_SUPPORT_TBTAB_UNWIND) because findFunctionName() would also be used for GCC on AIX but GCC does not use the traceback table for unwinding.

libunwind/src/UnwindCursor.hpp
2104

Good catch! Fixed.

2113

Good catch! Fixed.

libunwind/src/Unwind_AIXExtras.cpp
21

This source file Unwind_AIXExtras.cpp won't be included for non-AIX targets as specified in file CMakeLists.txt. Also, the code inside the file is guarded by #if defined(_AIX) in line 11.

xingxue updated this revision to Diff 399424.Jan 12 2022, 12:48 PM
xingxue marked 5 inline comments as done.

Addressed comments.

  • fix errors in comments.
compnerd added inline comments.Jan 31 2022, 9:43 AM
libunwind/include/__libunwind_config.h
65 ↗(On Diff #399424)

Should we consider just padding on clang instead of having a 1-entry difference on clang and gcc?

libunwind/src/Registers.hpp
612

Please prefer the surrounding area's formatting rather than git-clang-format.

1180

Similar.

libunwind/src/UnwindCursor.hpp
1245

Prefer a C++ style cast - reinterpret_cast<tbtable *>(_info.unwind_info).

1324

Please prefer a C++style cast: reinterpret_cast<uintptr_t>(_info.extra);.

1984

C++ style casts please and throughout this change.

xingxue updated this revision to Diff 405413.Feb 2 2022, 2:29 PM
xingxue marked 6 inline comments as done.
xingxue edited the summary of this revision. (Show Details)

Addressed comments:

  • Remove the cursor size definition for building with AIX GCC for now because it is not enabled yet;
  • Use the C++ style cast rather than C style cast;
  • Be consistent with the surrounding area's formatting.
libunwind/include/__libunwind_config.h
65 ↗(On Diff #399424)

Currently the libc++/libc++abi/libunwind build scripts only support building on AIX with clang and the AIX CMake does not work well with gcc. I am deferring this change for gcc to when there is a need to build with gcc on AIX. Please let me know if you have concerns.

libunwind/src/Registers.hpp
612

Fixed, thanks!

1180

Fixed, thanks!

libunwind/src/UnwindCursor.hpp
1245

Fixed, thanks!

1324

Fixed, thanks!

1984

Fixed, thanks!

Ping... This patch failed the pre-merge check because the formatting in lines 611-612 and 1179-1180 of file Registers.hpp follows the surrounding code rather than git-clang-format.

Gentle ping.

Herald added a project: Restricted Project. · View Herald TranscriptMar 21 2022, 12:53 PM
Herald added a subscriber: StephenFan. · View Herald Transcript

Gentle ping.

MaskRay accepted this revision.Apr 9 2022, 12:19 PM

LGTM. Realized that you may need someone in the #libunwind

(Sorry for the belated response. I was mostly on trips and spent little time on catching up patches.)

libunwind/src/assembly.h
212

Can #elif be changed to #else?

libunwind/src/config.h
63–69

The re-indentation seems unneeded). You may just ignore the clang-foramt diagnostic.

If we want to do formatting, it should be a separate change; I believe the problem is that .clang-format does not precisely reflect the PPIndentWidth the code base actually uses.

This revision is now accepted and ready to land.Apr 9 2022, 12:19 PM
This revision was landed with ongoing or failed builds.Apr 13 2022, 8:04 AM
This revision was automatically updated to reflect the committed changes.
xingxue marked 2 inline comments as done.Apr 13 2022, 8:53 AM

@MaskRay @compnerd @cebowleratibm @sfertile Thanks so much for your time and effort in reviewing this patch and providing constructive suggestions to make it better! Much appreciated!

libunwind/src/assembly.h
212

Good suggestion! Changed in add-on commit rG9c0152cda35f.

libunwind/src/config.h
63–69

Good point, reversed the indentation in add-on commit rG9c0152cda35f.