Page MenuHomePhabricator

[lldb] Fix Recognizer/assert.test with glibc-2.33.9000-31.fc35.x86_64
ClosedPublic

Authored by jankratochvil on Jun 29 2021, 10:13 AM.

Details

Summary

While on regular Linux system (Fedora 34 GA, not updated):

* thread #1, name = '1', stop reason = hit program assert
    frame #0: 0x00007ffff7e242a2 libc.so.6`raise + 322
    frame #1: 0x00007ffff7e0d8a4 libc.so.6`abort + 278
    frame #2: 0x00007ffff7e0d789 libc.so.6`__assert_fail_base.cold + 15
    frame #3: 0x00007ffff7e1ca16 libc.so.6`__assert_fail + 70
  * frame #4: 0x00000000004011bd 1`main at assert.c:7:3

On Fedora 35 pre-release one gets:

* thread #1, name = '1', stop reason = signal SIGABRT
  * frame #0: 0x00007ffff7e48ee3 libc.so.6`pthread_kill@GLIBC_2.2.5 + 67
    frame #1: 0x00007ffff7dfb986 libc.so.6`raise + 22
    frame #2: 0x00007ffff7de5806 libc.so.6`abort + 230
    frame #3: 0x00007ffff7de571b libc.so.6`__assert_fail_base.cold + 15
    frame #4: 0x00007ffff7df4646 libc.so.6`__assert_fail + 70
    frame #5: 0x00000000004011bd 1`main at assert.c:7:3

I did not write a testcase as one needs the specific glibc. An artificial test would just copy the changed source.

Diff Detail

Event Timeline

jankratochvil created this revision.Jun 29 2021, 10:13 AM
jankratochvil requested review of this revision.Jun 29 2021, 10:13 AM
fweimer added inline comments.
lldb/source/Target/AssertFrameRecognizer.cpp
52

The symbol version is architecture-dependent (not every architecture uses GLIBC_2.2.5), and there is also another GLIBC_2.34 symbol version even on x86-64.

jankratochvil planned changes to this revision.Jun 29 2021, 11:08 AM
mib added a comment.EditedJun 29 2021, 11:10 AM

Another possibility could be to add a bool is_regex to the SymbolLocation struct and set it to true for Linux, then in RegisterAssertFrameRecognizer if that flag is set, build a RegularExpression out of the every symbol string in the struct and call the FrameRecognizerManager::AddRecognizer overload that uses RegularExpressionSP arguments.

That'd allow you to have some flexibility with library versions while only registering 1 recognizer. I think using regexes could also make it future-proof.

jankratochvil edited the summary of this revision. (Show Details)
jankratochvil added inline comments.
lldb/source/Target/AssertFrameRecognizer.cpp
115

There would be nice llvm::join. But it wants Elem.size() while ConstString here provides Elem.GetLength(). I did not try to extend StringExtras.h.

mib accepted this revision.Jun 29 2021, 12:26 PM

LGTM!

lldb/source/Target/AssertFrameRecognizer.cpp
115

That's unfortunate indeed.

This revision is now accepted and ready to land.Jun 29 2021, 12:26 PM
jankratochvil planned changes to this revision.Jun 29 2021, 12:26 PM

Unfortunately AddRecognizer requires both or none module and symbols as regexes. And we cannot make module a regex as GetFileSpec().FileEquals would not work then. So I have just escaped the regex.
It looks a bit fishy as StackFrameRecognizer uses entry.module_regexp->Execute ignoring case insensitivity on OSX but then I do not target that platform myself.

This revision is now accepted and ready to land.Jun 29 2021, 12:34 PM
jankratochvil requested review of this revision.Jun 29 2021, 12:34 PM
jankratochvil marked 2 inline comments as done.
mib accepted this revision.Jun 30 2021, 3:32 PM
This revision is now accepted and ready to land.Jun 30 2021, 3:32 PM
This revision was landed with ongoing or failed builds.Jul 1 2021, 12:16 AM
This revision was automatically updated to reflect the committed changes.

Thanks for the review.

mib added a comment.Jul 1 2021, 2:54 AM

Thanks for the review.

No problem! Thank you for the patch!