[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

[Luboš Luňák via Phabricator via lldb-commits]

llunak added a comment.

In D86417#2239062 , @JosephTremoulet 
wrote:

> and let me know how that goes?  I don't have a linux system here to try it 
> on, but I have a feeling this may fix the problem you're seeing.

Yes, it fixes my problem. However (if I'm getting it right), it fixes it by 
fixing only 'raise',  but not 'abort'. If I do 'image show-unwind -n abort', I 
still get:

  eh_frame UnwindPlan:
  This UnwindPlan originally sourced from eh_frame CFI
  This UnwindPlan is sourced from the compiler: yes.
  This UnwindPlan is valid at all instruction locations: no.
  This UnwindPlan is for a trap handler function: no.
  ...
  eh_frame augmented UnwindPlan:
  This UnwindPlan originally sourced from eh_frame CFI
  This UnwindPlan is sourced from the compiler: yes.
  This UnwindPlan is valid at all instruction locations: no.
  This UnwindPlan is for a trap handler function: yes.

The augmented unwind plan for abort is still for a trap handler (your patch 
affects "plus augmentation from assembly parsing", which is not this unwind 
plan). I don't know if this is a problem, but it looks suspicious to me, 
especially given that the eh_frame unwind plan is not for a trap handler.

In D86417#2239062 , @JosephTremoulet 
wrote:

> From memory, I'd have expected that for `__restore_rt`, but not for `raise` 
> or `abort`.  Can you dump the augmentation fields from the CIEs (or 
> instrument `FDEToUnwindPlan`) to verify we're getting that right?

I made FDEToUnwindPlan() to print the augmentation string and it contains 'S' 
just once, when "GetSymbolOrFunctionName(m_sym_ctx)" in 
RegisterContextUnwind::InitializeNonZerothFrame() says "__restore_rt". So I 
assume that means the info in the binary is correct.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D86417

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

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

[Luboš Luňák via Phabricator via lldb-commits]

llunak updated this revision to Diff 288780.
llunak edited the summary of this revision.
llunak added a comment.

And it indeed was suspicious. 3fd917d8860e9bdcabc14c536da4377307906be0 didn't 
update the UnwindPlan copy ctor to copy the field.

This patch fixes the problem even without your change.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D86417

Files:
  lldb/include/lldb/Symbol/UnwindPlan.h


Index: lldb/include/lldb/Symbol/UnwindPlan.h
===
--- lldb/include/lldb/Symbol/UnwindPlan.h
+++ lldb/include/lldb/Symbol/UnwindPlan.h
@@ -393,6 +393,7 @@
 m_plan_is_sourced_from_compiler(rhs.m_plan_is_sourced_from_compiler),
 m_plan_is_valid_at_all_instruction_locations(
 rhs.m_plan_is_valid_at_all_instruction_locations),
+m_plan_is_for_signal_trap(rhs.m_plan_is_for_signal_trap),
 m_lsda_address(rhs.m_lsda_address),
 m_personality_func_addr(rhs.m_personality_func_addr) {
 m_row_list.reserve(rhs.m_row_list.size());


Index: lldb/include/lldb/Symbol/UnwindPlan.h
===
--- lldb/include/lldb/Symbol/UnwindPlan.h
+++ lldb/include/lldb/Symbol/UnwindPlan.h
@@ -393,6 +393,7 @@
 m_plan_is_sourced_from_compiler(rhs.m_plan_is_sourced_from_compiler),
 m_plan_is_valid_at_all_instruction_locations(
 rhs.m_plan_is_valid_at_all_instruction_locations),
+m_plan_is_for_signal_trap(rhs.m_plan_is_for_signal_trap),
 m_lsda_address(rhs.m_lsda_address),
 m_personality_func_addr(rhs.m_personality_func_addr) {
 m_row_list.reserve(rhs.m_row_list.size());
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D85705: Add a "Trace" plug-in to LLDB to add process trace support in stages.

[walter erquinigo via Phabricator via lldb-commits]

wallace updated this revision to Diff 288788.
wallace added a comment.

- Removed whitespace only changes.
- Simplified the way a SettingsParser is created, now there's no need to 
passinga a pointer around.
- Fixed the forward declaration in lldb-private-interfaces.

This should address all comments so far except for the llvm::json changes. I'm 
okay with creating another patch with those changes if the reviewers prefer 
that. They are simple, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D85705

Files:
  lldb/include/lldb/Core/PluginManager.h
  lldb/include/lldb/Target/Trace.h
  lldb/include/lldb/Target/TraceSettingsParser.h
  lldb/include/lldb/lldb-forward.h
  lldb/include/lldb/lldb-private-interfaces.h
  lldb/source/Commands/CMakeLists.txt
  lldb/source/Commands/CommandObjectTrace.cpp
  lldb/source/Commands/CommandObjectTrace.h
  lldb/source/Commands/Options.td
  lldb/source/Core/PluginManager.cpp
  lldb/source/Interpreter/CommandInterpreter.cpp
  lldb/source/Plugins/CMakeLists.txt
  lldb/source/Plugins/Trace/CMakeLists.txt
  lldb/source/Plugins/Trace/intel-pt/CMakeLists.txt
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPT.h
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.cpp
  lldb/source/Plugins/Trace/intel-pt/TraceIntelPTSettingsParser.h
  lldb/source/Target/CMakeLists.txt
  lldb/source/Target/Trace.cpp
  lldb/source/Target/TraceSettingsParser.cpp
  lldb/source/Utility/StructuredData.cpp
  lldb/test/API/commands/trace/TestTraceLoad.py
  lldb/test/API/commands/trace/TestTraceSchema.py
  lldb/test/API/commands/trace/intelpt-trace/3842849.trace
  lldb/test/API/commands/trace/intelpt-trace/a.out
  lldb/test/API/commands/trace/intelpt-trace/main.cpp
  lldb/test/API/commands/trace/intelpt-trace/trace.json
  lldb/test/API/commands/trace/intelpt-trace/trace_bad.json
  lldb/test/API/commands/trace/intelpt-trace/trace_bad2.json
  lldb/test/API/commands/trace/intelpt-trace/trace_bad3.json
  llvm/include/llvm/Support/JSON.h
  llvm/lib/Support/JSON.cpp

Index: llvm/lib/Support/JSON.cpp
===
--- llvm/lib/Support/JSON.cpp
+++ llvm/lib/Support/JSON.cpp
@@ -77,6 +77,62 @@
 return V->getAsArray();
   return nullptr;
 }
+
+llvm::Error Object::createMissingKeyError(llvm::StringRef K) const {
+  std::string str;
+  llvm::raw_string_ostream OS(str);
+  json::Object obj = *this;
+  OS << llvm::formatv(
+  "JSON object is missing the \"{0}\" field.\nValue::\n{1:2}", K,
+  json::Value(std::move(obj)));
+  OS.flush();
+
+  return llvm::createStringError(std::errc::invalid_argument, OS.str().c_str());
+}
+
+llvm::Expected Object::getOrError(StringRef K) const {
+  if (const json::Value *value = get(K))
+return value;
+  else
+return createMissingKeyError(K);
+}
+
+llvm::Expected Object::getIntegerOrError(StringRef K) const {
+  if (llvm::Expected V = getOrError(K))
+return V.get()->getAsIntegerOrError();
+  else
+return V.takeError();
+}
+
+llvm::Expected Object::getStringOrError(StringRef K) const {
+  if (llvm::Expected V = getOrError(K))
+return V.get()->getAsStringOrError();
+  else
+return V.takeError();
+}
+
+llvm::Expected Object::getArrayOrError(StringRef K) const {
+  if (llvm::Expected V = getOrError(K))
+return V.get()->getAsArrayOrError();
+  else
+return V.takeError();
+}
+
+llvm::Expected
+Object::getObjectOrError(StringRef K) const {
+  if (llvm::Expected V = getOrError(K))
+return V.get()->getAsObjectOrError();
+  else
+return V.takeError();
+}
+
+llvm::Expected>
+Object::getOptionalStringOrError(StringRef K) const {
+  if (const json::Value *V = get(K))
+return V->getAsStringOrError();
+  return llvm::None;
+}
+
 bool operator==(const Object &LHS, const Object &RHS) {
   if (LHS.size() != RHS.size())
 return false;
@@ -150,6 +206,42 @@
   }
 }
 
+llvm::Error Value::createWrongTypeError(llvm::StringRef type) const {
+  std::string str;
+  llvm::raw_string_ostream OS(str);
+  OS << llvm::formatv("JSON value is expected to be \"{0}\".\nValue:\n{1:2}",
+  type, *this);
+  OS.flush();
+
+  return llvm::createStringError(std::errc::invalid_argument, OS.str().c_str());
+}
+
+llvm::Expected Value::getAsIntegerOrError() const {
+  llvm::Optional value = getAsInteger();
+  if (value.hasValue())
+return *value;
+  return createWrongTypeError("integer");
+}
+
+llvm::Expected Value::getAsStringOrError() const {
+  llvm::Optional value = getAsString();
+  if (value.hasValue())
+return *value;
+  return createWrongTypeError("string");
+}
+
+llvm::Expected Value::getAsArrayOrError() const {
+  if (const json::Array *value = getAsArray())
+return value;
+  return createWrongTypeError("array");
+}
+
+llvm::Expected Value::getAsObjectOrError() const {
+  if (const json::Object *value = getAsObject())
+return v

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Ah, so we've still got a bug in 
x86AssemblyInspectionEngine::AugmentUnwindPlanFromCallSite somehow for abort(). 
 This method takes an UnwindPlan from eh_frame instructions (which says it is 
not a trap handler - correct), and adds some epilogue unwind rows at the end if 
needed and sets a couple of flags.  I'm not sure how the trap handler lazybool 
is getting set to True in the process of this conversion. :/  I'll read through 
this method more closely Monday and try to spot how that is happening.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D86417

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

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

[Luboš Luňák via Phabricator via lldb-commits]

llunak added a comment.

No, everything's fine :). See the updated patch. The UnwindPlan object already 
enters x86AssemblyInspectionEngine::AugmentUnwindPlanFromCallSite() with the 
lazybool uninitialized and fixing that fixes everything:

  (lldb) bt
  * thread #1, name = 'a.out', stop reason = breakpoint 1.1
* frame #0: 0x004006bb a.out`handler(sig=6) at main.c:7:5
  frame #1: 0x77a555a0 libc.so.6`__restore_rt
  frame #2: 0x77a55520 libc.so.6`raise + 272
  frame #3: 0x77a56b01 libc.so.6`abort + 337
  frame #4: 0x004006e9 a.out`abort_caller at main.c:12:5
  frame #5: 0x00400743 a.out`main at main.c:23:5
  frame #6: 0x77a4034a libc.so.6`__libc_start_main + 234
  frame #7: 0x004005fa a.out`_start at start.S:120
  (lldb) image show-unwind -n raise
  UNWIND PLANS for libc.so.6`raise (start addr 0x77a55410)
  
  Asynchronous (not restricted to call-sites) UnwindPlan is 'eh_frame CFI plus 
augmentation from assembly parsing'
  Synchronous (restricted to call-sites) UnwindPlan is 'eh_frame CFI'
  
  Assembly language inspection UnwindPlan:
  This UnwindPlan originally sourced from assembly insn profiling
  This UnwindPlan is sourced from the compiler: no.
  This UnwindPlan is valid at all instruction locations: yes.
  This UnwindPlan is for a trap handler function: no.
  Address range of this UnwindPlan: [libc.so.6.PT_LOAD[0]..text + 
88224-0x000159e3)
  row[0]:0: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8] 
  row[1]:1: CFA=rsp+16 => rbx=[CFA-16] rsp=CFA+0 rip=[CFA-8] 
  row[2]:   25: CFA=rsp+288 => rbx=[CFA-16] rsp=CFA+0 rip=[CFA-8] 
  row[3]:  301: CFA=rsp+16 => rbx=[CFA-16] rsp=CFA+0 rip=[CFA-8] 
  row[4]:  302: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8] 
  row[5]:  303: CFA=rsp+288 => rbx=[CFA-16] rsp=CFA+0 rip=[CFA-8] 
  
  eh_frame UnwindPlan:
  This UnwindPlan originally sourced from eh_frame CFI
  This UnwindPlan is sourced from the compiler: yes.
  This UnwindPlan is valid at all instruction locations: no.
  This UnwindPlan is for a trap handler function: no.
  Address range of this UnwindPlan: [libc.so.6.PT_LOAD[0]..text + 
88224-0x000159e3)
  row[0]:0: CFA=rsp +8 => rip=[CFA-8] 
  row[1]:1: CFA=rsp+16 => rbx=[CFA-16] rip=[CFA-8] 
  row[2]:   25: CFA=rsp+288 => rbx=[CFA-16] rip=[CFA-8] 
  row[3]:  301: CFA=rsp+16 => rbx=[CFA-16] rip=[CFA-8] 
  row[4]:  302: CFA=rsp +8 => rbx=[CFA-16] rip=[CFA-8] 
  row[5]:  304: CFA=rsp+288 => rbx=[CFA-16] rip=[CFA-8] 
  
  eh_frame augmented UnwindPlan:
  This UnwindPlan originally sourced from eh_frame CFI plus augmentation from 
assembly parsing
  This UnwindPlan is sourced from the compiler: no.
  This UnwindPlan is valid at all instruction locations: yes.
  This UnwindPlan is for a trap handler function: no.
  Address range of this UnwindPlan: [libc.so.6.PT_LOAD[0]..text + 
88224-0x000159e3)
  row[0]:0: CFA=rsp +8 => rip=[CFA-8] 
  row[1]:1: CFA=rsp+16 => rbx=[CFA-16] rip=[CFA-8] 
  row[2]:   25: CFA=rsp+288 => rbx=[CFA-16] rip=[CFA-8] 
  row[3]:  301: CFA=rsp+16 => rbx=[CFA-16] rip=[CFA-8] 
  row[4]:  302: CFA=rsp +8 => rbx=[CFA-16] rip=[CFA-8] 
  row[5]:  304: CFA=rsp+288 => rbx=[CFA-16] rip=[CFA-8] 
  
  Arch default UnwindPlan:
  This UnwindPlan originally sourced from x86_64 default unwind plan
  This UnwindPlan is sourced from the compiler: no.
  This UnwindPlan is valid at all instruction locations: no.
  This UnwindPlan is for a trap handler function: no.
  row[0]:0: CFA=rbp+16 => rbp=[CFA-16] rsp=CFA+0 rip=[CFA-8] 
  
  Arch default at entry point UnwindPlan:
  This UnwindPlan originally sourced from x86_64 at-func-entry default
  This UnwindPlan is sourced from the compiler: no.
  This UnwindPlan is valid at all instruction locations: not specified.
  This UnwindPlan is for a trap handler function: not specified.
  row[0]:0: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8] 
  
  
  (lldb) image show-unwind -n abort
  UNWIND PLANS for libc.so.6`abort (start addr 0x77a569b0)
  
  Asynchronous (not restricted to call-sites) UnwindPlan is 'eh_frame CFI'
  Synchronous (restricted to call-sites) UnwindPlan is 'eh_frame CFI'
  
  Assembly language inspection UnwindPlan:
  This UnwindPlan originally sourced from assembly insn profiling
  This UnwindPlan is sourced from the compiler: no.
  This UnwindPlan is valid at all instruction locations: yes.
  This UnwindPlan is for a trap handler function: no.
  Address range of this UnwindPlan: [libc.so.6.PT_LOAD[0]..text + 
93760-0x00017090)
  row[0]:0: CFA=rsp +8 => rsp=CFA+0 rip=[CFA-8] 
  row[1]:7: CFA=rsp+304 => rsp=CFA+0 rip=[CFA-8] 
  row[2]:   93: CFA=rsp+432 => rsp=CFA+0 rip=[CFA-8] 
  row[3]:  105: CFA=rsp+304 => rsp=CFA+0 rip=[CFA-8] 
  row[4]:  315: CFA=rsp+432 => rsp=CFA+0 rip=[CFA-8] 
  row[5]:  327: CFA=rsp+304 => rsp=CFA+0 rip=[CFA-8] 
  row[6]:  406: CFA=rsp+432 => rsp=CFA+0 rip=[CFA-8] 
  row[7]:  418: CFA=rsp+304 => rsp=CFA+0 rip=[CFA-8] 
  
  eh_frame Unw

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

[Jason Molenda via Phabricator via lldb-commits]

jasonmolenda added a comment.

Hahaha I remember looking at this constructor and expecting to find an 
uninitialized ivar, but it was initialized correctly and I went to look for 
another place where the bug might be.  I think I see why there's some confusion 
-- this change landed Thrusday:

commit 642cb7865f35ad7dbac78d716fcddaff561e8639 

Author: Benjamin Kramer 
Date:   Wed Aug 26 13:25:41 2020 +0200

  Copy m_plan_is_for_signal_trap member.
  
  Otherwise it would stay uninitialized. Found by msan.

doing exactly that.  When I was looking at the sources, that fix was already 
present.  I think we can close this phab?  I'm glad we would have found & fixed 
it even if Benjamin hadn't found it with msan. :)


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D86417

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

[Lldb-commits] [PATCH] D86417: [lldb] do not propagate eTrapHandlerFrame repeatedly

[Luboš Luňák via Phabricator via lldb-commits]

llunak abandoned this revision.
llunak added a comment.

Funny coincidence, the ctor has been broken for 5 years and that commit 
introducing my problem has been there for a year. Ok, closing, thank you for 
your help.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D86417

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