Fix+re-enable Assert StackFrame Recognizer on Linux

[jankratochvil]

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.

[mib]

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

In D73303#1846281, @labath wrote:

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.

[jankratochvil]

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.

[mib]

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

[jankratochvil]

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

[mib]

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.

[jankratochvil]

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]

In D74252#1865496, @jankratochvil wrote:

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 ...

[jankratochvil]

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]

In D74252#1865501, @jankratochvil wrote:

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]

LGTM! Thanks for the patch!

[jankratochvil]

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]

Hello Jan,

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

[jankratochvil]

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.

[JDevlieghere]

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.

[jankratochvil]

@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]

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]

lldb/source/Target/AssertFrameRecognizer.cpp

147

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

[aprantl]

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.

lldb/source/Target/AssertFrameRecognizer.cpp

 
 #include "lldb/Target/AssertFrameRecognizer.h"
 
 using namespace llvm;
 using namespace lldb;
 using namespace lldb_private;
 
 namespace lldb_private {
-/// Checkes if the module containing a symbol has debug info.
-///
-/// \param[in] target
-///    The target containing the module.
-/// \param[in] module_spec
-///    The module spec that should contain the symbol.
-/// \param[in] symbol_name
-///    The symbol's name that should be contained in the debug info.
-/// \return
-///    If  \b true the symbol was found, \b false otherwise.
-bool ModuleHasDebugInfo(Target &target, FileSpec &module_spec,
-                        StringRef symbol_name) {
-  ModuleSP module_sp = target.GetImages().FindFirstModule(module_spec);
-
-  if (!module_sp)
-    return false;
-
-  return module_sp->FindFirstSymbolWithNameAndType(ConstString(symbol_name));
-}
-
 /// Fetches the abort frame location depending on the current platform.
 ///
 /// \param[in] process_sp
 ///    The process that is currently aborting. This will give us information on
 ///    the target and the platform.
 /// \return
 ///    If the platform is supported, returns an optional tuple containing
 ///    the abort module as a \a FileSpec and the symbol name as a \a StringRef.
 ///    Otherwise, returns \a llvm::None.
 llvm::Optional<std::tuple<FileSpec, StringRef>>
 GetAbortLocation(Process *process) {
   Target &target = process->GetTarget();
 
   FileSpec module_spec;
   StringRef symbol_name;
 
   switch (target.GetArchitecture().GetTriple().getOS()) {
   case llvm::Triple::Darwin:
   case llvm::Triple::MacOSX:
     module_spec = FileSpec("libsystem_kernel.dylib");
     symbol_name = "__pthread_kill";
     break;
   case llvm::Triple::Linux:
     module_spec = FileSpec("libc.so.6");
-    symbol_name = "__GI_raise";
-    if (!ModuleHasDebugInfo(target, module_spec, symbol_name))
-      symbol_name = "raise";
     break;
   default:
     Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_UNWIND));
     LLDB_LOG(log, "AssertFrameRecognizer::GetAbortLocation Unsupported OS");
     return llvm::None;
   }
 
   return std::make_tuple(module_spec, symbol_name);
GetAssertLocation(Process *process) {
   switch (target.GetArchitecture().GetTriple().getOS()) {
   case llvm::Triple::Darwin:
   case llvm::Triple::MacOSX:
     module_spec = FileSpec("libsystem_c.dylib");
     symbol_name = "__assert_rtn";
     break;
   case llvm::Triple::Linux:
     module_spec = FileSpec("libc.so.6");
-    symbol_name = "__GI___assert_fail";
-    if (!ModuleHasDebugInfo(target, module_spec, symbol_name))
-      symbol_name = "__assert_fail";
     break;
   default:
     Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_UNWIND));
     LLDB_LOG(log, "AssertFrameRecognizer::GetAssertLocation Unsupported OS");
     return llvm::None;
   }
 
   return std::make_tuple(module_spec, symbol_name);
for (uint32_t frame_index = 0; frame_index < frames_to_fetch; frame_index++) {
     SymbolContext sym_ctx =
         prev_frame_sp->GetSymbolContext(eSymbolContextEverything);
 
     if (sym_ctx.module_sp->GetFileSpec().FileEquals(module_spec) &&
         sym_ctx.GetFunctionName() == ConstString(function_name)) {
 
       // We go a frame beyond the assert location because the most relevant
       // frame for the user is the one in which the assert function was called.
       // If the assert location is the last frame fetched, then it is set as
       // the most relevant frame.
 
       StackFrameSP most_relevant_frame_sp = thread_sp->GetStackFrameAtIndex(
           std::min(frame_index + 1, last_frame_index));
 
       // Pass assert location to AbortRecognizedStackFrame to set as most
       // relevant frame.
       return lldb::RecognizedStackFrameSP(

lldb/test/Shell/Recognizer/assert.test

-# UNSUPPORTED: system-windows, system-linux
+# UNSUPPORTED: system-windows
 # RUN: %clang_host -g -O0 %S/Inputs/assert.c -o %t.out
 # RUN: %lldb -b -s %s %t.out | FileCheck %s
 run
 # CHECK: thread #{{.*}}stop reason = hit program assert
 frame info
 # CHECK: frame #{{.*}}`main at assert.c
 frame recognizer info 0
 # CHECK: frame 0 is recognized by Assert StackFrame Recognizer
 set set thread-format "{${thread.stop-reason-raw}}\n"
 thread info
 # CHECK: signal SIGABRT
 q