This is an archive of the discontinued LLVM Phabricator instance.

Fix+re-enable Assert StackFrame Recognizer on Linux
ClosedPublic

Authored by jankratochvil on Feb 7 2020, 12:38 PM.

Details

Summary

D73303 was failing on Fedora Linux and so it was disabled by Skip the AssertFrameRecognizer test for Linux.
I find no easy way how to find out if it gets recognized as __assert_fail or __GI___assert_fail as when ModuleHasDebugInfo gets called libc.so.6 is not yet loaded by the debuggee.
DWARF symbol __GI___assert_fail overrides the ELF symbol __assert_fail.
While external debug info (=DWARF) gets disabled for testsuite (D55859) that sure does not apply for real world usage.

Diff Detail

Event Timeline

jankratochvil created this revision.Feb 7 2020, 12:38 PM
mib accepted this revision.Feb 7 2020, 1:08 PM

Originally, when I implemented the patch and tested it on linux, the symbol on the abort frame was (GI_assert_fail) on my machine.

Since the failure reason was not very obvious from the bot message, I took a quick look, and to save you the trouble of reproducing this, here's what I found:

The reason the new test failed was because the symbols you are searching for (__GI_raise, __GI___assert_fail) are private libc symbols which are only available if you happen to have debug info for the system libc installed. If you don't, these will show up as raise and __assert_fail, respectively.

This is why I thought I'd need to support both.

Otherwise, LGTM!

Thanks for investigating the issue.

lldb/source/Target/AssertFrameRecognizer.cpp
19–38

In this case, we can also remove this.

63–64

And also this if global bindings are preferred.

This revision is now accepted and ready to land.Feb 7 2020, 1:08 PM
jankratochvil marked 2 inline comments as done.Feb 7 2020, 1:22 PM
In D74252#1864879, @mib wrote:

Originally, when I implemented the patch and tested it on linux, the symbol on the abort frame was (GI_assert_fail) on my machine.

Maybe you did not have settings set symbols.enable-external-lookup false like the testsuite. Or you used LLDB before Jan 13th (D63540).

Thanks for verifying it, going to land it.

This revision was automatically updated to reflect the committed changes.
mib added a comment.Feb 7 2020, 1:31 PM

I used an apt installed lldb so it was probably a build older than Jan 13th.

I have reverted this change as it fixed the testsuite but real world usage was broken:

(lldb) run
assert.test.tmp.out: /home/jkratoch/redhat/llvm-monorepo/lldb/test/Shell/Recognizer/Inputs/assert.c:7: int main(): Assertion `a == 42' failed.
Process 12062 stopped
* thread #1, name = 'assert.test.tmp', stop reason = signal SIGABRT
    frame #0: 0x00007ffff7ddee35 libc.so.6`__GI_raise(sig=2) at raise.c:51:1
   48     __libc_signal_restore_set (&set);
   49
   50     return ret;
-> 51   }
   52   libc_hidden_def (raise)
   53   weak_alias (raise, gsignal)

I think it really needs to compare both variants of the function name on Linux.
Both variants get mapped name->address but it is difficult to say which one will get looked up address->name.
Otherwise one could also fix why DWARF resolves the __GI_* variants from:

<4d4b>   DW_AT_linkage_name: (indirect string, offset: 0x1b784): __GI___assert_fail
<4d4f>   DW_AT_name        : (indirect string, offset: 0x1b789): __assert_fail
jankratochvil retitled this revision from Fix+re-enable Assert StackFrame Recognizer to Fix+re-enable Assert StackFrame Recognizer on Linux.
jankratochvil edited the summary of this revision. (Show Details)
jankratochvil reopened this revision.Feb 8 2020, 1:49 AM
This revision is now accepted and ready to land.Feb 8 2020, 1:49 AM
jankratochvil requested review of this revision.Feb 8 2020, 1:49 AM
mib added a comment.Feb 8 2020, 2:10 AM

We'd like to avoid registering recognizers multiple times for the same purpose ...

This could cause some issues down the road, since those recognizers can - indirectly - perform an action (change the current frame or the stop reason).

Also, from a usability stand-point, this could be confusing for the user when listing recognizers.

In D74252#1865493, @mib wrote:

We'd like to avoid registering recognizers multiple times for the same purpose ...

OK, then it could be changed to a regex. Is it fine afterwards?

mib added a comment.Feb 8 2020, 2:27 AM

OK, then it could be changed to a regex. Is it fine afterwards?

Knowing that this will be called every time a thread stops, it would be better if we could avoid processing a regex every time we try to recognise a frame. But if that's the only way to solve the issue ...

In D74252#1865500, @mib wrote:

Knowing that this will be called every time a thread stops, it would be better if we could avoid processing a regex every time we try to recognise a frame.

Or StackFrameRecognizerManagerImpl::AddRecognizer and its m_recognizers can have ConstString symbol1, symbol2;. What is less bad?

mib added a comment.EditedFeb 8 2020, 2:44 AM
In D74252#1865500, @mib wrote:

Knowing that this will be called every time a thread stops, it would be better if we could avoid processing a regex every time we try to recognise a frame.

Or StackFrameRecognizerManagerImpl::AddRecognizer and its m_recognizers can have ConstString symbol1, symbol2;. What is less bad?

This sounds like a good idea, but let's leave symbol as is and add a ConstString alternate_symbol in StackFrameRecognizerManagerImpl::RegisteredEntry defaulting to an empty ConstString

mib accepted this revision.Feb 8 2020, 3:34 AM

LGTM! Thanks for the patch!

This revision is now accepted and ready to land.Feb 8 2020, 3:34 AM
In D74252#1865504, @mib wrote:

but let's leave symbol as is and add a ConstString alternate_symbol

FYI this had a purpose. Any code touching symbol should be rechecked if it should not handle also alternate_symbol now. Both in existing codebase and also in any 3rd party patches we do not know.

mib added a comment.Feb 10 2020, 1:19 AM

Hello Jan,

Has the latest version of this patch landed yet or does it need some extra work ?

In D74252#1866476, @mib wrote:

Has the latest version of this patch landed yet or does it need some extra work ?

I was giving some time to other reviewers but it is checked-in now.

This revision was automatically updated to reflect the committed changes.
JDevlieghere added inline comments.Feb 10 2020, 9:34 AM
lldb/source/Commands/CommandObjectFrame.cpp
966 ↗(On Diff #243480)

We should stream to the output stream directly to avoid all these useless conversions.

lldb/source/Target/AssertFrameRecognizer.cpp
28

With two elements in the tuple I was torn between a struct with named fields, but with 3 fields I strongly believe that would be the better choice.

147

I believe someone added an overload for comparing ConstStrings with StringRefs. We shouldn't have to pay the price of creating one here just for comparison. Same below.

@davide Sorry for the revert rG6b2979c12300b90a1e69791d43ee9cff14f4265e - I will find some way to test stuff on OSX, I already made too many OSX breakages recently.

teemperor added inline comments.
lldb/source/Target/AssertFrameRecognizer.cpp
147

I really don't want a == overload for StringRef in ConstString. The *only* reason for using ConstString is using less memory for duplicate strings and being fast to compare against other ConstStrings. So if we add an overload for comparing against StringRef then code like this will look really innocent even though it essentially goes against the idea of ConstString. Either both string lists are using ConstString or neither does (which I would prefer). But having a list of normal strings and comparing it against ConstStrings usually means that the design is kinda flawed. Then we have both normal string comparisons but also the whole overhead of ConstString.

Looking at this patch we already construct at one point a ConstString from the function name at one point, so that might as well be a ConstString in the first place. If we had an implicit comparison than the conversion here would look really innocent and no one would notice.

JDevlieghere added inline comments.Feb 12 2020, 11:15 AM
lldb/source/Target/AssertFrameRecognizer.cpp
147

Hmm, you're totally right, I must be confusing this with something else then.

aprantl added inline comments.Feb 13 2020, 10:14 AM
lldb/source/Target/AssertFrameRecognizer.cpp
147

The *only* reason for using ConstString is using less memory for duplicate strings and being fast to compare against other ConstStrings.

Just for completeness, a huge use-case for ConstString in LLDB are as keys in DenseMaps and sorted vectors. This avoids redundantly string them in multiple StringMaps.