[Lldb-commits] [PATCH] D93052: "target create" shouldn't save target if the command failed

[Tatyana Krasnukha via Phabricator via lldb-commits]

tatyana-krasnukha updated this revision to Diff 311128.
tatyana-krasnukha added a comment.

Removed `do_select`'s default value.


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

https://reviews.llvm.org/D93052

Files:
  lldb/include/lldb/Target/TargetList.h
  lldb/source/API/SBDebugger.cpp
  lldb/source/Commands/CommandObjectProcess.cpp
  lldb/source/Commands/CommandObjectTarget.cpp
  lldb/source/Plugins/Platform/POSIX/PlatformPOSIX.cpp
  lldb/source/Plugins/Platform/Windows/PlatformWindows.cpp
  lldb/source/Plugins/Platform/gdb-server/PlatformRemoteGDBServer.cpp
  lldb/source/Target/Platform.cpp
  lldb/source/Target/TargetList.cpp
  lldb/source/Target/TraceSessionFileParser.cpp
  lldb/unittests/Process/ProcessEventDataTest.cpp
  lldb/unittests/Thread/ThreadTest.cpp

Index: lldb/unittests/Thread/ThreadTest.cpp
===
--- lldb/unittests/Thread/ThreadTest.cpp
+++ lldb/unittests/Thread/ThreadTest.cpp
@@ -83,16 +83,11 @@
 } // namespace
 
 TargetSP CreateTarget(DebuggerSP &debugger_sp, ArchSpec &arch) {
-  Status error;
   PlatformSP platform_sp;
   TargetSP target_sp;
-  error = debugger_sp->GetTargetList().CreateTarget(
+  debugger_sp->GetTargetList().CreateTarget(
   *debugger_sp, "", arch, eLoadDependentsNo, platform_sp, target_sp);
 
-  if (target_sp) {
-debugger_sp->GetTargetList().SetSelectedTarget(target_sp.get());
-  }
-
   return target_sp;
 }
 
Index: lldb/unittests/Process/ProcessEventDataTest.cpp
===
--- lldb/unittests/Process/ProcessEventDataTest.cpp
+++ lldb/unittests/Process/ProcessEventDataTest.cpp
@@ -111,16 +111,11 @@
 typedef std::shared_ptr EventSP;
 
 TargetSP CreateTarget(DebuggerSP &debugger_sp, ArchSpec &arch) {
-  Status error;
   PlatformSP platform_sp;
   TargetSP target_sp;
-  error = debugger_sp->GetTargetList().CreateTarget(
+  debugger_sp->GetTargetList().CreateTarget(
   *debugger_sp, "", arch, eLoadDependentsNo, platform_sp, target_sp);
 
-  if (target_sp) {
-debugger_sp->GetTargetList().SetSelectedTarget(target_sp.get());
-  }
-
   return target_sp;
 }
 
Index: lldb/source/Target/TraceSessionFileParser.cpp
===
--- lldb/source/Target/TraceSessionFileParser.cpp
+++ lldb/source/Target/TraceSessionFileParser.cpp
@@ -123,8 +123,6 @@
   ParsedProcess parsed_process;
   parsed_process.target_sp = target_sp;
 
-  m_debugger.GetTargetList().SetSelectedTarget(target_sp.get());
-
   ProcessSP process_sp = target_sp->CreateProcess(
   /*listener*/ nullptr, "trace",
   /*crash_file*/ nullptr,
Index: lldb/source/Target/TargetList.cpp
===
--- lldb/source/Target/TargetList.cpp
+++ lldb/source/Target/TargetList.cpp
@@ -48,9 +48,13 @@
 LoadDependentFiles load_dependent_files,
 const OptionGroupPlatform *platform_options,
 TargetSP &target_sp) {
-  return CreateTargetInternal(debugger, user_exe_path, triple_str,
-  load_dependent_files, platform_options,
-  target_sp);
+  auto result = TargetList::CreateTargetInternal(
+  debugger, user_exe_path, triple_str, load_dependent_files,
+  platform_options, target_sp);
+
+  if (target_sp && result.Success())
+AddTargetInternal(target_sp, /*do_select*/ true);
+  return result;
 }
 
 Status TargetList::CreateTarget(Debugger &debugger,
@@ -58,8 +62,13 @@
 const ArchSpec &specified_arch,
 LoadDependentFiles load_dependent_files,
 PlatformSP &platform_sp, TargetSP &target_sp) {
-  return CreateTargetInternal(debugger, user_exe_path, specified_arch,
-  load_dependent_files, platform_sp, target_sp);
+  auto result = TargetList::CreateTargetInternal(
+  debugger, user_exe_path, specified_arch, load_dependent_files,
+  platform_sp, target_sp);
+
+  if (target_sp && result.Success())
+AddTargetInternal(target_sp, /*do_select*/ true);
+  return result;
 }
 
 Status TargetList::CreateTargetInternal(
@@ -388,9 +397,6 @@
 target_sp->AppendExecutableSearchPaths(file_dir);
   }
 
-  std::lock_guard guard(m_target_list_mutex);
-  m_selected_target_idx = m_target_list.size();
-  m_target_list.push_back(target_sp);
   // Now prime this from the dummy target:
   target_sp->PrimeFromDummyTarget(debugger.GetDummyTarget());
 
@@ -552,18 +558,29 @@
   return UINT32_MAX;
 }
 
-uint32_t TargetList::SetSelectedTarget(Target *target) {
+void TargetList::AddTargetInternal(TargetSP target_sp, bool do_select) {
+  lldbassert(std::find(m_target_list.begin(), m_target_list.end(), target_sp) ==
+ m_target_list.end() &&
+ "target already exists it the list");
+  m_targe

[Lldb-commits] [PATCH] D92187: [lldb] [POSIX-DYLD] Add libraries from initial rendezvous brkpt hit

[Michał Górny via Phabricator via lldb-commits]

mgorny updated this revision to Diff 311191.
mgorny added a comment.

Updated to unload duplicate ld.so as suggested by @labath.


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

https://reviews.llvm.org/D92187

Files:
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
  lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
  lldb/source/Plugins/Process/FreeBSDRemote/NativeProcessFreeBSD.cpp
  lldb/test/API/api/multithreaded/TestMultithreaded.py
  
lldb/test/API/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py
  lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
  lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
  
lldb/test/API/tools/lldb-server/libraries-svr4/TestGdbRemoteLibrariesSvr4Support.py
  lldb/test/Shell/Unwind/thread-step-out-ret-addr-check.test

Index: lldb/test/Shell/Unwind/thread-step-out-ret-addr-check.test
===
--- lldb/test/Shell/Unwind/thread-step-out-ret-addr-check.test
+++ lldb/test/Shell/Unwind/thread-step-out-ret-addr-check.test
@@ -3,7 +3,6 @@
 
 # REQUIRES: target-x86_64
 # UNSUPPORTED: system-windows
-# XFAIL: system-freebsd
 
 # RUN: %clang_host %p/Inputs/call-asm.c -x assembler-with-cpp %p/Inputs/thread-step-out-ret-addr-check.s -o %t
 # RUN: not %lldb %t -s %s -b 2>&1 | FileCheck %s
Index: lldb/test/API/tools/lldb-server/libraries-svr4/TestGdbRemoteLibrariesSvr4Support.py
===
--- lldb/test/API/tools/lldb-server/libraries-svr4/TestGdbRemoteLibrariesSvr4Support.py
+++ lldb/test/API/tools/lldb-server/libraries-svr4/TestGdbRemoteLibrariesSvr4Support.py
@@ -120,7 +120,7 @@
 
 @llgs_test
 @skipUnlessPlatform(["linux", "android", "freebsd", "netbsd"])
-@expectedFailureAll(oslist=["freebsd", "netbsd"])
+@expectedFailureNetBSD
 def test_libraries_svr4_load_addr(self):
 self.setup_test()
 self.libraries_svr4_has_correct_load_addr()
Index: lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
===
--- lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
+++ lldb/test/API/tools/lldb-server/TestLldbGdbServer.py
@@ -843,7 +843,6 @@
 self.qMemoryRegionInfo_is_supported()
 
 @llgs_test
-@expectedFailureAll(oslist=["freebsd"])
 def test_qMemoryRegionInfo_is_supported_llgs(self):
 self.init_llgs_test()
 self.build()
@@ -908,7 +907,6 @@
 self.qMemoryRegionInfo_reports_code_address_as_executable()
 
 @skipIfWindows # No pty support to test any inferior output
-@expectedFailureAll(oslist=["freebsd"])
 @llgs_test
 def test_qMemoryRegionInfo_reports_code_address_as_executable_llgs(self):
 self.init_llgs_test()
@@ -975,7 +973,6 @@
 self.qMemoryRegionInfo_reports_stack_address_as_readable_writeable()
 
 @skipIfWindows # No pty support to test any inferior output
-@expectedFailureAll(oslist=["freebsd"])
 @llgs_test
 def test_qMemoryRegionInfo_reports_stack_address_as_readable_writeable_llgs(
 self):
@@ -1042,7 +1039,6 @@
 self.qMemoryRegionInfo_reports_heap_address_as_readable_writeable()
 
 @skipIfWindows # No pty support to test any inferior output
-@expectedFailureAll(oslist=["freebsd"])
 @llgs_test
 def test_qMemoryRegionInfo_reports_heap_address_as_readable_writeable_llgs(
 self):
Index: lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
===
--- lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
+++ lldb/test/API/functionalities/memory-region/TestMemoryRegion.py
@@ -23,7 +23,6 @@
 'main.cpp',
 '// Run here before printing memory regions')
 
-@expectedFailureAll(oslist=["freebsd"])
 def test(self):
 self.build()
 
Index: lldb/test/API/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py
===
--- lldb/test/API/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py
+++ lldb/test/API/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py
@@ -15,7 +15,6 @@
 mydir = TestBase.compute_mydir(__file__)
 NO_DEBUG_INFO_TESTCASE = True
 
-@expectedFailureAll(oslist=["freebsd"], bugnumber='llvm.org/pr48373')
 @expectedFailureNetBSD
 def test(self):
 self.build()
Index: lldb/test/API/api/multithreaded/TestMultithreaded.py
===
--- lldb/test/API/api/multithreaded/TestMultithreaded.py
+++ lldb/test/API/api/multithreaded/TestMultithreaded.py
@@ -31,7 +31,6 @@
 @skipIfNoSBHeaders
 # clang-cl does not support throw or catch (llvm.org/pr24538)
 @skipIfWindows
-@expectedFailure

[Lldb-commits] [PATCH] D92957: ExpressionParser: Migrate to FileEntryRef in ParseInternal, NFC

[Jan Svoboda via Phabricator via lldb-commits]

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

LGTM.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92957

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

[Lldb-commits] [PATCH] D92164: Make CommandInterpreter's execution context the same as debugger's one.

[Jim Ingham via Phabricator via lldb-commits]

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

LGTM - thanks for doing this!


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

https://reviews.llvm.org/D92164

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

[Lldb-commits] [PATCH] D93052: "target create" shouldn't save target if the command failed

[Jim Ingham via Phabricator via lldb-commits]

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

LGTM, thanks for the cleanup.


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

https://reviews.llvm.org/D93052

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

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

> And with these changes together, breaking on the function breaks on line 5 
> (presumably because this is creating the constant used by the second '&&' 
> which is on line 5) instead of line 3. Not the worst thing, but I imagine 
> given Sony's interest in less "jumpy" line tables, this might not be super 
> desirable.

It's breaking on an `xorl %eax,%eax` which is produced by the PHI at the end of 
the conditional expression, which now has the source location of the operator 
at the top of the expression tree, which is the second `&&` operator, yeah.  
Not the best.  (FTR, the jumpiness complaints we get are usually more 
egregious, hopping between different source statements somewhat arbitrarily; 
not sure anyone would complain too loudly about hopping around within an 
expression.  We see less of that in any case, because we suppress column info, 
but still.)

The real bear here is getting that constant to behave reasonably.  In all cases 
the sequence of instructions is the same, but silly things keep happening to 
the line table.  (As an aside, GCC does this entire thing with control-flow, 
not trying to compute and then test the bool result of the expression.  They 
also handle `a--` better.  But I digress.)

Current HEAD puts the xor in the prologue (which also puts the while-loop top 
*before* prologue_end, that can't be right).  Adding the PHI change doesn't 
affect this, because FastISel is still materializing the zero in the local 
value map, which (in HEAD) has no source locations.

  .LBB0_1:# %while.cond
xorl%eax, %eax
  .Ltmp0:
.loc1 3 10 prologue_end # multi-line-while.c:3:10
cmpl$0, -4(%rbp)

With just my FastISel change (D91734 ), 
prologue_end is better, but the xor has line-0, which makes gdb sad (it decides 
the entire function has no source locations, which is wrong, but whatever):

  .LBB0_1:# %while.cond
  .Ltmp0:
.loc1 0 0 prologue_end  # multi-line-while.c:0:0
xorl%eax, %eax
.loc1 3 10  # multi-line-while.c:3:10
cmpl$0, -4(%rbp)

With the PHI change from D92606 , plus D91734 
, we see the PHI taking effect, which means we 
first break on line 5 and then hop backwards:

  .LBB0_1:# %while.cond
  .Ltmp0:
.loc1 5 10 prologue_end # multi-line-while.c:5:10
xorl%eax, %eax
.loc1 3 10  # multi-line-while.c:3:10
cmpl$0, -4(%rbp)

I think we'd prefer something more like this:

  .LBB0_1:# %while.cond
  .Ltmp0:
.loc1 3 10 prologue_end # multi-line-while.c:3:10
xorl%eax, %eax
cmpl$0, -4(%rbp)

Getting that to happen means not making the PHI change, and persuading FastISel 
to do something more intelligent.  Flushing on every instruction did seem to 
help, maybe if we made sure that first instruction also had the source location 
of the first "real" instruction?  I will experiment with this tomorrow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91734

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

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

[David Blaikie via Phabricator via lldb-commits]

dblaikie added a comment.

In D91734#2443231 , @probinson wrote:

> In D91734#2432988 , @dblaikie wrote:
>
>> Yeah, it boils down to something like this:
>>
>>   void f1(const char*, const char*) { }
>>   int main() {
>> char prog[1];
>>   
>> f1(prog,
>>prog);
>>   }
>>
>> Without the patch, you step to the 'f1' line, and then step into or over the 
>> function call (step or next).
>> But with these patches applied - you step to the 'f1(' line, then the 
>> 'prog);' line, then back to the 'f1(' line, then into/over the function call.
>>
>> Again, could be acceptable - if those arguments had specific non-trivial 
>> code in them (like other function calls) you'd certainly get that step 
>> forward/backward behavior - but it's notewhorthy that this is change would 
>> make more cases like that & it'd be good to understand why/if that's a good 
>> thing overall.
>
> If you dump the IR you should see two GEP instructions, one for each actual 
> parameter expression.  Prior to the patch, FastISel found the second one 
> computed the same value as the first one, and reused the value (in this case, 
> the result of `lea`).  This is one of the 5% of cases where a value is being 
> reused across instructions, with the HEAD compiler. After the patch, we flush 
> the local map after each GEP, so the second one gets its own separate `lea` 
> (with its own source location).  So, you're stepping to the first actual, 
> then to the second, then to the call.  Hope that answers the "why" question.
>
> Whether it's a "good" thing... that path is fraught with peril.  Looking at 
> it somewhat abstractly (as I am sometimes wont to do), Clang is making 
> choices to associate the source location of each parameter with the IR 
> instructions to compute that parameter, and by flushing the map on every IR 
> instruction, LLVM is more faithfully representing that in the final object.  
> This is -O0 after all, where a more faithful representation is more likely to 
> be more debuggable.  gcc is making entirely different choices, lumping all 
> parameter-prep instructions in with the function call itself.  I'm hard 
> pressed to say one is better than the other; they are different.  Coke and 
> Pepsi; people may have preferences, but that's not the same as being higher 
> on a good-ness scale.

Fair enough - if you're satisfied these are reasonable cases of 
different-but-reasonable DWARF locations for the instructions, I'm OK with it.

If possible, would be good if you could run the gdb test suite and document the 
"regressions" (and possibly document test case changes you'd make to keep those 
tests passing) with this change - if you're liable to do that sooner or later 
anyway for your own test infrastructure. Otherwise I'll just handle it on our 
end.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91734

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

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

[Paul Robinson via Phabricator via lldb-commits]

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

Reopening, will upload a new diff in a moment that modifies how PHIs are 
handled.  I intend to run the gdb suite on this, will post findings when I have 
them.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91734

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

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

[Paul Robinson via Phabricator via lldb-commits]

probinson updated this revision to Diff 310935.
probinson edited the summary of this revision.
probinson added a reviewer: dblaikie.
probinson added a comment.

Rebase and combine cf1cc774d and dc35368c 
, plus 
revise handling constants materialized due to PHIs.
I plan to do some build-time and object size measurements, similar to the first 
time around.
I plan to run this against the gdb test suite, or at least the local copy we 
have here at Sony, and post my findings.

I'm remembering that in a number of the tests, I had to add improvements to the 
use of -LABEL so I could track down the issues.  Those improvements should 
probably be factored out into a separate NFC commit as they're not strictly 
part of this change.  I'll get to that later (but obviously before committing 
this).


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

https://reviews.llvm.org/D91734

Files:
  lld/test/wasm/debug-removed-fn.ll
  lldb/test/Shell/SymbolFile/NativePDB/disassembly.cpp
  lldb/test/Shell/SymbolFile/NativePDB/load-pdb.cpp
  llvm/include/llvm/CodeGen/FastISel.h
  llvm/lib/CodeGen/SelectionDAG/FastISel.cpp
  llvm/test/CodeGen/AArch64/arm64-abi_align.ll
  llvm/test/CodeGen/AArch64/arm64-fast-isel-call.ll
  llvm/test/CodeGen/AArch64/arm64-fast-isel-gv.ll
  llvm/test/CodeGen/AArch64/arm64-fast-isel-intrinsic.ll
  llvm/test/CodeGen/AArch64/arm64-fast-isel.ll
  llvm/test/CodeGen/AArch64/arm64-patchpoint-webkit_jscc.ll
  llvm/test/CodeGen/AArch64/cfguard-checks.ll
  llvm/test/CodeGen/AArch64/elf-globals-static.ll
  llvm/test/CodeGen/AArch64/large-stack.ll
  llvm/test/CodeGen/ARM/fast-isel-call.ll
  llvm/test/CodeGen/ARM/fast-isel-intrinsic.ll
  llvm/test/CodeGen/ARM/fast-isel-ldr-str-thumb-neg-index.ll
  llvm/test/CodeGen/ARM/fast-isel-ldrh-strh-arm.ll
  llvm/test/CodeGen/ARM/fast-isel-select.ll
  llvm/test/CodeGen/ARM/fast-isel.ll
  llvm/test/CodeGen/Mips/Fast-ISel/callabi.ll
  llvm/test/CodeGen/Mips/Fast-ISel/fastalloca.ll
  llvm/test/CodeGen/Mips/Fast-ISel/fpcmpa.ll
  llvm/test/CodeGen/Mips/Fast-ISel/icmpa.ll
  llvm/test/CodeGen/Mips/Fast-ISel/logopm.ll
  llvm/test/CodeGen/Mips/Fast-ISel/overflt.ll
  llvm/test/CodeGen/Mips/Fast-ISel/shftopm.ll
  llvm/test/CodeGen/Mips/Fast-ISel/simplestore.ll
  llvm/test/CodeGen/Mips/Fast-ISel/simplestorei.ll
  llvm/test/CodeGen/Mips/emergency-spill-slot-near-fp.ll
  llvm/test/CodeGen/PowerPC/elf-common.ll
  llvm/test/CodeGen/PowerPC/fast-isel-load-store.ll
  llvm/test/CodeGen/PowerPC/mcm-1.ll
  llvm/test/CodeGen/PowerPC/mcm-13.ll
  llvm/test/CodeGen/PowerPC/mcm-2.ll
  llvm/test/CodeGen/PowerPC/mcm-3.ll
  llvm/test/CodeGen/PowerPC/mcm-6.ll
  llvm/test/CodeGen/PowerPC/mcm-9.ll
  llvm/test/CodeGen/PowerPC/mcm-default.ll
  llvm/test/CodeGen/X86/atomic-unordered.ll
  llvm/test/CodeGen/X86/atomic64.ll
  llvm/test/CodeGen/X86/crash-O0.ll
  llvm/test/CodeGen/X86/fast-isel-constant.ll
  llvm/test/CodeGen/X86/fast-isel-mem.ll
  llvm/test/CodeGen/X86/fast-isel-select.ll
  llvm/test/CodeGen/X86/lvi-hardening-loads.ll
  llvm/test/CodeGen/X86/membarrier.ll
  llvm/test/CodeGen/X86/pr32241.ll
  llvm/test/CodeGen/X86/pr32256.ll
  llvm/test/CodeGen/X86/pr32284.ll
  llvm/test/CodeGen/X86/pr32340.ll
  llvm/test/CodeGen/X86/pr44749.ll
  llvm/test/CodeGen/X86/volatile.ll
  llvm/test/DebugInfo/COFF/lines-bb-start.ll
  llvm/test/DebugInfo/Mips/delay-slot.ll
  llvm/test/DebugInfo/X86/fission-ranges.ll

Index: llvm/test/DebugInfo/X86/fission-ranges.ll
===
--- llvm/test/DebugInfo/X86/fission-ranges.ll
+++ llvm/test/DebugInfo/X86/fission-ranges.ll
@@ -45,31 +45,31 @@
 ; if they've changed due to a bugfix, change in register allocation, etc.
 
 ; CHECK:  [[A]]:
-; CHECK-NEXT:   DW_LLE_startx_length (0x0002, 0x000f): DW_OP_consts +0, DW_OP_stack_value
-; CHECK-NEXT:   DW_LLE_startx_length (0x0003, 0x000b): DW_OP_reg0 RAX
-; CHECK-NEXT:   DW_LLE_startx_length (0x0004, 0x0012): DW_OP_breg7 RSP-4
+; CHECK-NEXT:   DW_LLE_startx_length (0x0001, 0x0011): DW_OP_consts +0, DW_OP_stack_value
+; CHECK-NEXT:   DW_LLE_startx_length (0x0002, 0x000b): DW_OP_reg0 RAX
+; CHECK-NEXT:   DW_LLE_startx_length (0x0003, 0x0012): DW_OP_breg7 RSP-4
 ; CHECK-NEXT:   DW_LLE_end_of_list   ()
 ; CHECK:  [[E]]:
-; CHECK-NEXT:   DW_LLE_startx_length (0x0005, 0x000b): DW_OP_reg0 RAX
-; CHECK-NEXT:   DW_LLE_startx_length (0x0006, 0x005a): DW_OP_breg7 RSP-48
+; CHECK-NEXT:   DW_LLE_startx_length (0x0004, 0x000b): DW_OP_reg0 RAX
+; CHECK-NEXT:   DW_LLE_startx_length (0x0005, 0x005a): DW_OP_breg7 RSP-48
 ; CHECK-NEXT:   DW_LLE_end_of_list   ()
 ; CHECK:  [[B]]:
-; CHECK-NEXT:   DW_LLE_startx_length (0x0007, 0x000b): DW_OP_reg0 RAX
-; CHECK-NEXT:   DW_LLE_startx_length (0x0008, 0x0042): DW_OP_breg7 RSP-24
+; CHECK-NEXT:   DW_LLE_startx_length (0x0006, 0x000b): DW_OP_reg0 RAX
+; CHECK-NE

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

[Paul Robinson via Phabricator via lldb-commits]

probinson requested review of this revision.
probinson added a comment.

This is still showing as approved; I'm going to try "Request Review" to see if 
that helps.


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

https://reviews.llvm.org/D91734

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

[Lldb-commits] [PATCH] D91734: [FastISel] Flush local value map on every instruction

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

In D91734#2432988 , @dblaikie wrote:

> Yeah, it boils down to something like this:
>
>   void f1(const char*, const char*) { }
>   int main() {
> char prog[1];
>   
> f1(prog,
>prog);
>   }
>
> Without the patch, you step to the 'f1' line, and then step into or over the 
> function call (step or next).
> But with these patches applied - you step to the 'f1(' line, then the 
> 'prog);' line, then back to the 'f1(' line, then into/over the function call.
>
> Again, could be acceptable - if those arguments had specific non-trivial code 
> in them (like other function calls) you'd certainly get that step 
> forward/backward behavior - but it's notewhorthy that this is change would 
> make more cases like that & it'd be good to understand why/if that's a good 
> thing overall.

If you dump the IR you should see two GEP instructions, one for each actual 
parameter expression.  Prior to the patch, FastISel found the second one 
computed the same value as the first one, and reused the value (in this case, 
the result of `lea`).  This is one of the 5% of cases where a value is being 
reused across instructions, with the HEAD compiler. After the patch, we flush 
the local map after each GEP, so the second one gets its own separate `lea` 
(with its own source location).  So, you're stepping to the first actual, then 
to the second, then to the call.  Hope that answers the "why" question.

Whether it's a "good" thing... that path is fraught with peril.  Looking at it 
somewhat abstractly (as I am sometimes wont to do), Clang is making choices to 
associate the source location of each parameter with the IR instructions to 
compute that parameter, and by flushing the map on every IR instruction, LLVM 
is more faithfully representing that in the final object.  This is -O0 after 
all, where a more faithful representation is more likely to be more debuggable. 
 gcc is making entirely different choices, lumping all parameter-prep 
instructions in with the function call itself.  I'm hard pressed to say one is 
better than the other; they are different.  Coke and Pepsi; people may have 
preferences, but that's not the same as being higher on a good-ness scale.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D91734

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