[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Adrian Prantl via Phabricator via lldb-commits]

aprantl added inline comments.



Comment at: lldb/source/Target/AssertFrameRecognizer.cpp:152
+ConstString func_name = sym_ctx.GetFunctionName();
+if (func_name == ConstString(function_name) ||
+alternate_function_name.empty() ||

JDevlieghere wrote:
> teemperor wrote:
> > JDevlieghere wrote:
> > > 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.
> > 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.
> Hmm, you're totally right, I must be confusing this with something else then. 
> 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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Target/AssertFrameRecognizer.cpp:152
+ConstString func_name = sym_ctx.GetFunctionName();
+if (func_name == ConstString(function_name) ||
+alternate_function_name.empty() ||

teemperor wrote:
> JDevlieghere wrote:
> > 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.
> 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.
Hmm, you're totally right, I must be confusing this with something else then. 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Raphael Isemann via Phabricator via lldb-commits]

teemperor added inline comments.



Comment at: lldb/source/Target/AssertFrameRecognizer.cpp:152
+ConstString func_name = sym_ctx.GetFunctionName();
+if (func_name == ConstString(function_name) ||
+alternate_function_name.empty() ||

JDevlieghere wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a subscriber: davide.
jankratochvil added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Jonas Devlieghere via Phabricator via lldb-commits]

JDevlieghere added inline comments.



Comment at: lldb/source/Commands/CommandObjectFrame.cpp:966
 name = "(internal)";
   result.GetOutputStream().Printf(
+  "%d: %s, module %s, function %s{%s}%s\n", recognizer_id,

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



Comment at: lldb/source/Target/AssertFrameRecognizer.cpp:29
 ///Otherwise, returns \a llvm::None.
-llvm::Optional>
+llvm::Optional>
 GetAbortLocation(Process *process) {

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. 



Comment at: lldb/source/Target/AssertFrameRecognizer.cpp:152
+ConstString func_name = sym_ctx.GetFunctionName();
+if (func_name == ConstString(function_name) ||
+alternate_function_name.empty() ||

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Jan Kratochvil via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG1a39f1b966a8: [lldb] Fix+re-enable Assert StackFrame 
Recognizer on Linux (authored by jankratochvil).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252

Files:
  lldb/include/lldb/Target/StackFrameRecognizer.h
  lldb/source/Commands/CommandObjectFrame.cpp
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
  lldb/source/Target/AssertFrameRecognizer.cpp
  lldb/source/Target/StackFrameRecognizer.cpp
  lldb/test/Shell/Recognizer/assert.test
  lldb/unittests/Target/StackFrameRecognizerTest.cpp

Index: lldb/unittests/Target/StackFrameRecognizerTest.cpp
===
--- lldb/unittests/Target/StackFrameRecognizerTest.cpp
+++ lldb/unittests/Target/StackFrameRecognizerTest.cpp
@@ -77,6 +77,7 @@
   StackFrameRecognizerManager::ForEach(
   [_printed](uint32_t recognizer_id, std::string name,
  std::string function, std::string symbol,
+ std::string alternate_symbol,
  bool regexp) { any_printed = true; });
 
   EXPECT_TRUE(any_printed);
Index: lldb/test/Shell/Recognizer/assert.test
===
--- lldb/test/Shell/Recognizer/assert.test
+++ lldb/test/Shell/Recognizer/assert.test
@@ -1,4 +1,4 @@
-# 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
Index: lldb/source/Target/StackFrameRecognizer.cpp
===
--- lldb/source/Target/StackFrameRecognizer.cpp
+++ lldb/source/Target/StackFrameRecognizer.cpp
@@ -50,24 +50,28 @@
 
 class StackFrameRecognizerManagerImpl {
 public:
-  void AddRecognizer(StackFrameRecognizerSP recognizer,
- ConstString module, ConstString symbol,
+  void AddRecognizer(StackFrameRecognizerSP recognizer, ConstString module,
+ ConstString symbol, ConstString alternate_symbol,
  bool first_instruction_only) {
-m_recognizers.push_front({(uint32_t)m_recognizers.size(), false, recognizer, false, module, RegularExpressionSP(),
-  symbol, RegularExpressionSP(),
+m_recognizers.push_front({(uint32_t)m_recognizers.size(), false, recognizer,
+  false, module, RegularExpressionSP(), symbol,
+  alternate_symbol, RegularExpressionSP(),
   first_instruction_only});
   }
 
   void AddRecognizer(StackFrameRecognizerSP recognizer,
  RegularExpressionSP module, RegularExpressionSP symbol,
  bool first_instruction_only) {
-m_recognizers.push_front({(uint32_t)m_recognizers.size(), false, recognizer, true, ConstString(), module,
+m_recognizers.push_front({(uint32_t)m_recognizers.size(), false, recognizer,
+  true, ConstString(), module, ConstString(),
   ConstString(), symbol, first_instruction_only});
   }
 
   void ForEach(
-  std::function const ) {
+  std::function const
+  ) {
 for (auto entry : m_recognizers) {
   if (entry.is_regexp) {
 std::string module_name;
@@ -79,11 +83,12 @@
   symbol_name = entry.symbol_regexp->GetText().str();
 
 callback(entry.recognizer_id, entry.recognizer->GetName(), module_name,
- symbol_name, true);
+ symbol_name, {}, true);
 
   } else {
-callback(entry.recognizer_id, entry.recognizer->GetName(), entry.module.GetCString(),
- entry.symbol.GetCString(), false);
+callback(entry.recognizer_id, entry.recognizer->GetName(),
+ entry.module.GetCString(), entry.symbol.GetCString(),
+ entry.alternate_symbol.GetCString(), false);
   }
 }
   }
@@ -120,7 +125,10 @@
 if (!entry.module_regexp->Execute(module_name.GetStringRef())) continue;
 
   if (entry.symbol)
-if (entry.symbol != function_name) continue;
+if (entry.symbol != function_name &&
+(!entry.alternate_symbol ||
+ entry.alternate_symbol != function_name))
+  continue;
 
   if (entry.symbol_regexp)
 if (!entry.symbol_regexp->Execute(function_name.GetStringRef()))
@@ -149,6 +157,7 @@
 ConstString module;
 RegularExpressionSP module_regexp;
 ConstString symbol;
+ConstString alternate_symbol;
 RegularExpressionSP symbol_regexp;
 bool first_instruction_only;
   };
@@ -163,10 +172,10 @@
 }
 
 void StackFrameRecognizerManager::AddRecognizer(
-StackFrameRecognizerSP recognizer, ConstString module,
-ConstString symbol, bool 

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

Hello Jan,

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Med Ismail Bennani via Phabricator via lldb-commits]

mib accepted this revision.
mib added a comment.
This revision is now accepted and ready to land.

LGTM! Thanks for the patch!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil updated this revision to Diff 243358.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252

Files:
  lldb/include/lldb/Target/StackFrameRecognizer.h
  lldb/source/Commands/CommandObjectFrame.cpp
  
lldb/source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCRuntimeV2.cpp
  lldb/source/Target/AssertFrameRecognizer.cpp
  lldb/source/Target/StackFrameRecognizer.cpp
  lldb/test/Shell/Recognizer/assert.test
  lldb/unittests/Target/StackFrameRecognizerTest.cpp

Index: lldb/unittests/Target/StackFrameRecognizerTest.cpp
===
--- lldb/unittests/Target/StackFrameRecognizerTest.cpp
+++ lldb/unittests/Target/StackFrameRecognizerTest.cpp
@@ -77,6 +77,7 @@
   StackFrameRecognizerManager::ForEach(
   [_printed](uint32_t recognizer_id, std::string name,
  std::string function, std::string symbol,
+ std::string alternate_symbol,
  bool regexp) { any_printed = true; });
 
   EXPECT_TRUE(any_printed);
Index: lldb/test/Shell/Recognizer/assert.test
===
--- lldb/test/Shell/Recognizer/assert.test
+++ lldb/test/Shell/Recognizer/assert.test
@@ -1,4 +1,4 @@
-# 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
Index: lldb/source/Target/StackFrameRecognizer.cpp
===
--- lldb/source/Target/StackFrameRecognizer.cpp
+++ lldb/source/Target/StackFrameRecognizer.cpp
@@ -50,24 +50,28 @@
 
 class StackFrameRecognizerManagerImpl {
 public:
-  void AddRecognizer(StackFrameRecognizerSP recognizer,
- ConstString module, ConstString symbol,
+  void AddRecognizer(StackFrameRecognizerSP recognizer, ConstString module,
+ ConstString symbol, ConstString alternate_symbol,
  bool first_instruction_only) {
-m_recognizers.push_front({(uint32_t)m_recognizers.size(), false, recognizer, false, module, RegularExpressionSP(),
-  symbol, RegularExpressionSP(),
+m_recognizers.push_front({(uint32_t)m_recognizers.size(), false, recognizer,
+  false, module, RegularExpressionSP(), symbol,
+  alternate_symbol, RegularExpressionSP(),
   first_instruction_only});
   }
 
   void AddRecognizer(StackFrameRecognizerSP recognizer,
  RegularExpressionSP module, RegularExpressionSP symbol,
  bool first_instruction_only) {
-m_recognizers.push_front({(uint32_t)m_recognizers.size(), false, recognizer, true, ConstString(), module,
+m_recognizers.push_front({(uint32_t)m_recognizers.size(), false, recognizer,
+  true, ConstString(), module, ConstString(),
   ConstString(), symbol, first_instruction_only});
   }
 
   void ForEach(
-  std::function const ) {
+  std::function const
+  ) {
 for (auto entry : m_recognizers) {
   if (entry.is_regexp) {
 std::string module_name;
@@ -79,11 +83,12 @@
   symbol_name = entry.symbol_regexp->GetText().str();
 
 callback(entry.recognizer_id, entry.recognizer->GetName(), module_name,
- symbol_name, true);
+ symbol_name, {}, true);
 
   } else {
-callback(entry.recognizer_id, entry.recognizer->GetName(), entry.module.GetCString(),
- entry.symbol.GetCString(), false);
+callback(entry.recognizer_id, entry.recognizer->GetName(),
+ entry.module.GetCString(), entry.symbol.GetCString(),
+ entry.alternate_symbol.GetCString(), false);
   }
 }
   }
@@ -120,7 +125,10 @@
 if (!entry.module_regexp->Execute(module_name.GetStringRef())) continue;
 
   if (entry.symbol)
-if (entry.symbol != function_name) continue;
+if (entry.symbol != function_name &&
+(!entry.alternate_symbol ||
+ entry.alternate_symbol != function_name))
+  continue;
 
   if (entry.symbol_regexp)
 if (!entry.symbol_regexp->Execute(function_name.GetStringRef()))
@@ -149,6 +157,7 @@
 ConstString module;
 RegularExpressionSP module_regexp;
 ConstString symbol;
+ConstString alternate_symbol;
 RegularExpressionSP symbol_regexp;
 bool first_instruction_only;
   };
@@ -163,10 +172,10 @@
 }
 
 void StackFrameRecognizerManager::AddRecognizer(
-StackFrameRecognizerSP recognizer, ConstString module,
-ConstString symbol, bool first_instruction_only) {
-  GetStackFrameRecognizerManagerImpl().AddRecognizer(recognizer, module, symbol,
- 

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

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` defaulting to an empty `ConstString`


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

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?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer on Linux

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil updated this revision to Diff 243353.
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.
Herald added a subscriber: jfb.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252

Files:
  lldb/source/Target/AssertFrameRecognizer.cpp
  lldb/test/Shell/Recognizer/assert.test

Index: lldb/test/Shell/Recognizer/assert.test
===
--- lldb/test/Shell/Recognizer/assert.test
+++ lldb/test/Shell/Recognizer/assert.test
@@ -1,4 +1,4 @@
-# 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
Index: lldb/source/Target/AssertFrameRecognizer.cpp
===
--- lldb/source/Target/AssertFrameRecognizer.cpp
+++ lldb/source/Target/AssertFrameRecognizer.cpp
@@ -16,26 +16,6 @@
 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 , FileSpec _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
@@ -43,26 +23,26 @@
 ///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.
+///the abort module as a \a FileSpec and two symbol names as two \a
+///StringRef. The second \a StringRef may be empty.
 ///Otherwise, returns \a llvm::None.
-llvm::Optional>
+llvm::Optional>
 GetAbortLocation(Process *process) {
   Target  = process->GetTarget();
 
   FileSpec module_spec;
-  StringRef symbol_name;
+  StringRef symbol_name1, symbol_name2;
 
   switch (target.GetArchitecture().GetTriple().getOS()) {
   case llvm::Triple::Darwin:
   case llvm::Triple::MacOSX:
 module_spec = FileSpec("libsystem_kernel.dylib");
-symbol_name = "__pthread_kill";
+symbol_name1 = "__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";
+symbol_name1 = "raise";
+symbol_name2 = "__GI_raise";
 break;
   default:
 Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_UNWIND));
@@ -70,7 +50,7 @@
 return llvm::None;
   }
 
-  return std::make_tuple(module_spec, symbol_name);
+  return std::make_tuple(module_spec, symbol_name1, symbol_name2);
 }
 
 /// Fetches the assert frame location depending on the current platform.
@@ -80,27 +60,26 @@
 ///the target and the platform.
 /// \return
 ///If the platform is supported, returns an optional tuple containing
-///the asserting frame module as  a \a FileSpec and the symbol name as a \a
-///StringRef.
+///the asserting frame module as a \a FileSpec and two possible symbol
+///names as two \a StringRef. The second \a StringRef may be empty.
 ///Otherwise, returns \a llvm::None.
-llvm::Optional>
+llvm::Optional>
 GetAssertLocation(Process *process) {
   Target  = process->GetTarget();
 
   FileSpec module_spec;
-  StringRef symbol_name;
+  StringRef symbol_name1, symbol_name2;
 
   switch (target.GetArchitecture().GetTriple().getOS()) {
   case llvm::Triple::Darwin:
   case llvm::Triple::MacOSX:
 module_spec = FileSpec("libsystem_c.dylib");
-symbol_name = "__assert_rtn";
+symbol_name1 = "__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";
+symbol_name1 = "__assert_fail";
+symbol_name2 = "__GI___assert_fail";
 break;
   default:
 Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_UNWIND));
@@ -108,7 +87,7 @@
 return llvm::None;
   }
 
-  return std::make_tuple(module_spec, symbol_name);
+  return std::make_tuple(module_spec, symbol_name1, symbol_name2);
 }
 
 void RegisterAssertFrameRecognizer(Process *process) {
@@ 

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil added a comment.

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: 0x77ddee35 libc.so.6`__GI_raise(sig=2) at raise.c:51:1
 48 __libc_signal_restore_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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer

[Med Ismail Bennani via Phabricator via lldb-commits]

mib added a comment.

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer

[Jan Kratochvil via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGcf1046c716b3: [lldb] Fix+re-enable Assert StackFrame 
Recognizer on Linux (authored by jankratochvil).

Changed prior to commit:
  https://reviews.llvm.org/D74252?vs=243266=243274#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252

Files:
  lldb/source/Target/AssertFrameRecognizer.cpp
  lldb/test/Shell/Recognizer/assert.test


Index: lldb/test/Shell/Recognizer/assert.test
===
--- lldb/test/Shell/Recognizer/assert.test
+++ lldb/test/Shell/Recognizer/assert.test
@@ -1,4 +1,4 @@
-# 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
Index: lldb/source/Target/AssertFrameRecognizer.cpp
===
--- lldb/source/Target/AssertFrameRecognizer.cpp
+++ lldb/source/Target/AssertFrameRecognizer.cpp
@@ -16,26 +16,6 @@
 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 , FileSpec _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
@@ -60,9 +40,7 @@
 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";
+symbol_name = "raise";
 break;
   default:
 Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_UNWIND));
@@ -98,9 +76,7 @@
 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";
+symbol_name = "__assert_fail";
 break;
   default:
 Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_UNWIND));


Index: lldb/test/Shell/Recognizer/assert.test
===
--- lldb/test/Shell/Recognizer/assert.test
+++ lldb/test/Shell/Recognizer/assert.test
@@ -1,4 +1,4 @@
-# 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
Index: lldb/source/Target/AssertFrameRecognizer.cpp
===
--- lldb/source/Target/AssertFrameRecognizer.cpp
+++ lldb/source/Target/AssertFrameRecognizer.cpp
@@ -16,26 +16,6 @@
 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 , FileSpec _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
@@ -60,9 +40,7 @@
 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";
+symbol_name = "raise";
 break;
   default:
 Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_UNWIND));
@@ -98,9 +76,7 @@
 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";
+symbol_name = "__assert_fail";
 break;
   default:
 Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_UNWIND));

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil marked 2 inline comments as done.
jankratochvil added a comment.

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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer

[Med Ismail Bennani via Phabricator via lldb-commits]

mib accepted this revision.
mib added a comment.
This revision is now accepted and ready to land.

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.




Comment at: lldb/source/Target/AssertFrameRecognizer.cpp:19-38
 /// 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

In this case, we can also remove this.



Comment at: lldb/source/Target/AssertFrameRecognizer.cpp:63-64
 module_spec = FileSpec("libc.so.6");
 symbol_name = "__GI_raise";
 if (!ModuleHasDebugInfo(target, module_spec, symbol_name))
   symbol_name = "raise";

And also this if global bindings are preferred.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74252/new/

https://reviews.llvm.org/D74252



___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D74252: Fix+re-enable Assert StackFrame Recognizer

[Jan Kratochvil via Phabricator via lldb-commits]

jankratochvil created this revision.
jankratochvil added reviewers: mib, JDevlieghere, labath.
jankratochvil added a project: LLDB.
Herald added a subscriber: aprantl.

D73303  was failing on Fedora Linux 
 and so it was disabled by Skip the 
AssertFrameRecognizer test for Linux 
.
On Fedora 30 x86_64 I have:

  $ readelf -Ws /lib64/libc.so.6 |grep '^Symbol\|.*assert_fail'
  Symbol table '.dynsym' contains 2362 entries:
 630: 0003052070 FUNCGLOBAL DEFAULT   14 
__assert_fail@@GLIBC_2.2.5
  Symbol table '.symtab' contains 22711 entries:
 922: 0002275a15 FUNCLOCAL  DEFAULT   14 
__assert_fail_base.cold
   18044: 0003052070 FUNCLOCAL  DEFAULT   14 __GI___assert_fail
   20081: 000303a0   370 FUNCLOCAL  DEFAULT   14 __assert_fail_base
   21766: 0003052070 FUNCGLOBAL DEFAULT   14 __assert_fail

I do not see why your patch was ever expecting `__GI___assert_fail`:
`.symtab` can be present or not but that should not change that `__assert_fail` 
always wins - it is always present from `.dynsym` and it can never be overriden 
by `__GI___assert_fail` as `__GI___assert_fail` has only local binding. Global 
binding is preferred since D63540 .
External debug info symbols do not matter since D55859 
 (and DWARF should never be embedded in system 
`libc.so.6`).


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D74252

Files:
  lldb/source/Target/AssertFrameRecognizer.cpp
  lldb/test/Shell/Recognizer/assert.test


Index: lldb/test/Shell/Recognizer/assert.test
===
--- lldb/test/Shell/Recognizer/assert.test
+++ lldb/test/Shell/Recognizer/assert.test
@@ -1,4 +1,4 @@
-# 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
Index: lldb/source/Target/AssertFrameRecognizer.cpp
===
--- lldb/source/Target/AssertFrameRecognizer.cpp
+++ lldb/source/Target/AssertFrameRecognizer.cpp
@@ -98,9 +98,7 @@
 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";
+symbol_name = "__assert_fail";
 break;
   default:
 Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_UNWIND));


Index: lldb/test/Shell/Recognizer/assert.test
===
--- lldb/test/Shell/Recognizer/assert.test
+++ lldb/test/Shell/Recognizer/assert.test
@@ -1,4 +1,4 @@
-# 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
Index: lldb/source/Target/AssertFrameRecognizer.cpp
===
--- lldb/source/Target/AssertFrameRecognizer.cpp
+++ lldb/source/Target/AssertFrameRecognizer.cpp
@@ -98,9 +98,7 @@
 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";
+symbol_name = "__assert_fail";
 break;
   default:
 Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_UNWIND));
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits