[Lldb-commits] [PATCH] D145803: [clang][DebugInfo] Emit DW_AT_type of preferred name if available

[Paul Robinson via Phabricator via lldb-commits]

probinson added subscribers: wolfgangp, probinson.
probinson added a comment.

This is pretty different from the "always desugar to the canonical type" habit 
that has always been in place. Sony has done some downstream things to try to 
work around that in the past. @wolfgangp will remember it better than I do, but 
I think we make some effort to preserve the type-as-written. This goes in 
completely the other direction.




Comment at: clang/test/CodeGen/preferred_name.cpp:49
+
+Foo> varFooInt;
+

This doesn't become `Foo` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145803

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

[Lldb-commits] [PATCH] D140332: [ADT] Alias llvm::Optional to std::optional

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.
Herald added subscribers: jsetoain, JDevlieghere, ormris.

Worth trying on MSVC, which is the other more likely place to fail.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140332

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

[Lldb-commits] [PATCH] D139734: [LLDB] Remove redundant XFAIL

[Paul Robinson via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG5e6467bb3fd2: [lldb] Remove redundant XFAIL (authored by 
probinson).
Herald added a subscriber: lldb-commits.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139734

Files:
  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,3 @@
-# XFAIL: target-arm && linux-gnu
 # XFAIL: system-freebsd
 # XFAIL: system-netbsd
 #


Index: lldb/test/Shell/Recognizer/assert.test
===
--- lldb/test/Shell/Recognizer/assert.test
+++ lldb/test/Shell/Recognizer/assert.test
@@ -1,4 +1,3 @@
-# XFAIL: target-arm && linux-gnu
 # XFAIL: system-freebsd
 # XFAIL: system-netbsd
 #
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D131919: Move googletest to the third-party directory

[Paul Robinson via Phabricator via lldb-commits]

probinson accepted this revision.
probinson added a comment.

Yes, LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131919

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

[Lldb-commits] [PATCH] D137287: [Test] Fix CHECK typo.

[Paul Robinson via Phabricator via lldb-commits]

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

LGTM, but you'll want to be ready to jump on any bot failures.  That's just the 
nature of these kinds of changes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137287

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

[Lldb-commits] [PATCH] D137287: [Test] Fix CHECK typo.

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

Nice work!
have you verified that all the modified tests pass? naively it looks like you'd 
need at least 3 different targets to run them all (windows, webassembly, 
powerpc)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137287

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

[Lldb-commits] [PATCH] D131919: Move googletest to the third-party directory

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

In D131919#3749850 , @Meinersbur 
wrote:

> Semi-OT: `polly\lib\External` has 3 more third-party libraries. Two of them 
> have been heavily modified in-tree, the third has just a custom 
> CMakeLists.txt.
> Should these eventually also be moved?

I don't know that there's a formal policy.  Our copy of googletest is used by 
several projects, so unhooking it from the llvm tree has value in that regard.
Heavy modification might not be a blocker; googletest is somewhat 
stripped-down, although otherwise I think only lightly modified.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131919

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

[Lldb-commits] [PATCH] D131919: Move googletest to the third-party directory

[Paul Robinson via Phabricator via lldb-commits]

probinson added inline comments.



Comment at: clang/CMakeLists.txt:106
   AND EXISTS ${UNITTEST_DIR}/CMakeLists.txt)
 add_subdirectory(${UNITTEST_DIR} utils/unittest)
   endif()

Should this be `third-party/unittest` ? Looking at how the lldb cmake files 
were modified.



Comment at: lld/CMakeLists.txt:69
   AND EXISTS ${UNITTEST_DIR}/CMakeLists.txt)
 add_subdirectory(${UNITTEST_DIR} utils/unittest)
   endif()

See comment for clang/CMakeLists.txt



Comment at: mlir/CMakeLists.txt:25
   if(EXISTS ${UNITTEST_DIR}/googletest/include/gtest/gtest.h)
 add_subdirectory(${UNITTEST_DIR} utils/unittest)
   endif()

See comment for clang/CMakeLists.txt



Comment at: polly/CMakeLists.txt:37
 if (NOT TARGET gtest)
   add_subdirectory(${UNITTEST_DIR} utils/unittest)
 endif()

See comment for clang/CMakeLists.txt


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131919

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

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

[Paul Robinson via Phabricator via lldb-commits]

probinson added inline comments.



Comment at: clang/lib/Frontend/CompilerInvocation.cpp:1636
+if (Opts.getDebugInfo() == codegenoptions::DebugInfoConstructor)
+  Opts.setDebugInfo(codegenoptions::LimitedDebugInfo);
 

No... you want to check both options in one call, otherwise 
`-fno-use-ctor-homing -fuse-ctor-homing` will prefer the `no` version instead 
of last-one-wins.

I suggest fiddling the options should be done separately.
Also if you want to make it a clang option, the option name should have `debug` 
in it; pretty sure all the -f options related to debug info do that, and in any 
case it's a good idea.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106084

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

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

> Wouldn't the current "limited" behavior have problems for this shared 
> libraries situation too? Sounds like in that case -fstandalone-debug should 
> be used.

@jmorse am I remembering correctly, that we require dllimport-style 
annotations, so "limited" actually includes these types even if they aren't 
constructed locally?  I am vague on the details here.  But if ctor homing and 
limited both will consider a dllimport-ed type as requiring a full description, 
that's not a reason to pick one over the other.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106084

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

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

> (hence the renaming of "limited" a long time ago to "standalone-debug" to 
> create a policy/philosophy around what goes in each category).

Sorry, what?  I thought -fstandalone-debug meant FullDebugInfo, and AFAICT 
that's still how the driver handles it?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106084

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

[Lldb-commits] [PATCH] D106084: [DebugInfo] Switch to using constructor homing (-debug-info-kind=constructor) by default when debug info is enabled

[Paul Robinson via Phabricator via lldb-commits]

probinson added a reviewer: jmorse.
probinson added a subscriber: jmorse.
probinson added a comment.

+ @jmorse who is better placed than I am to say whether this is what Sony would 
prefer.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106084

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

[Lldb-commits] [PATCH] D105564: Fix for DWARF parsing to better handle auto return type for member functions

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

> Currently when we have a member function that has an auto return type and the 
> definition is out of line we generate two DWARF DIE.
> One for the declaration and a second one for the definition, the definition 
> holds the deduced type but does not contain a DW_AT_name nor
> a DW_AT_linkage_name so there was not way to look up the definition DIE.

Regarding the DWARF, the definition DIE should have a DW_AT_specification that 
points back to the declaration; is that missing here?
I'm not familiar with LLDB so it's likely I'm misunderstanding the problem.


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

https://reviews.llvm.org/D105564

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

This revision was automatically updated to reflect the committed changes.
Closed by commit rGc161775decdd: [FastISel] Flush local value map on every 
instruction (authored by probinson).
Herald added a subscriber: ormris.

Changed prior to commit:
  https://reviews.llvm.org/D91734?vs=314708&id=315817#toc

Repository:
  rG LLVM Github Monorepo

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-prolog-dbgloc.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-NEXT:   DW_LLE_startx_length (0x0007, 0x0042): DW_OP_breg7 RSP-24
 ; CHECK-NEXT:   DW_LLE_end_of_list   ()
 ; CHECK:  [[D]]:
-; CHECK-NEXT:   DW_LLE_startx_length (0x0009, 0x000b): DW_OP_reg0 RAX
-; CHECK-NEXT:   DW_LLE_startx_length (0x000a, 0x002a): DW_OP_breg7 RSP-12
+; CHECK-NEXT:   DW_LLE_startx_length (0x0008, 0x000b): DW_OP_reg0 RAX
+; CHECK-NEXT:   DW_LLE_startx_length (0x0009, 0x002a): DW_OP_breg7 RSP-12
 ; CHECK-NEXT

[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#2482318 , @dblaikie wrote:

> But, yes, we could possibly do better with more strategic is_stmt, but that's 
> a big/complex piece of work to tackle.

Oh, absolutely.  Didn't mean to imply otherwise.  And the behavior now is at 
least easily explainable in terms of source line.

Thanks for the review, this has been an irritant for our licensees since 
forever.  And especially thanks @rnk for the flush-on-every-instruction 
suggestion.


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#2480930 , @dblaikie wrote:

> I haven't looked closely, but I take it this one test modification seems 
> reasonable to you? I guess we're stepping into some subexpressions on a 
> function call that we previously didn't? (they didn't have any instructions 
> attributed to them until now, but it's not unreasonable that they have them 
> attributed - for materializing constants to pass to a function call when the 
> function call/constants are split over multiple lines, etc)

Exactly.  In each case, the test is trying to "next" over a function call; gcc 
attributes all parameter evaluation to the function name, while clang will 
attribute each parameter to its own location.  And when the parameters span 
multiple source lines, the is_stmt heuristic kicks in, so we stop on each line 
with actual parameters.

This is not ideal IMO; I'd rather be more principled about (a) stop at every 
parameter, or (b) stop at no parameters.  But without reworking how we do 
is_stmt, I think fiddling the test to do enough single-steps to actually get 
past the function call is okay.


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 314708.
probinson added a comment.

Change how PHIs are handled; if the operand has a debug location, use it, 
otherwise don't set a debug location.  Then, flushLocalValueMap() will look at 
the first local-value instruction; if it doesn't already have a debug location, 
we copy the location from the first instruction after the local-value 
instructions (which should have the debug location of the original IR 
instruction).


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-NEXT:   DW_LLE_startx_length (0x0007, 0x0042): DW_OP_breg7 RSP-24
 ; CHECK-NEXT:   DW_LLE_end_of_list   ()
 ; CHECK:  [[D]]:
-; CHECK-NEXT:   DW_LLE_startx_length (0x0009, 0x000b): DW_OP_reg0 RAX
-; CHECK-NEXT:   DW_LLE_startx_length (0x000a, 0x002a): DW_OP_breg7 RSP-12
+; CHECK-NEXT:   DW_LLE_startx_length (0x0008, 0x000b): DW_OP_reg0 RAX
+; CHECK-NEXT:   DW_LLE

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

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

This version of the patch avoids the weirdness I was seeing with prolog 
instructions in certain cases.

The gdb test suite is largely happy with this; it avoids the new failures I 
mentioned previously, and one test needed to have some "next" commands updated. 
 Here's the gdb test suite patch (against 8.3; I can provide more context if 
it's not clear how to apply to later versions).

  diff --git a/gdb-8.3/gdb/testsuite/gdb.base/foll-exec.exp 
b/gdb-8.3/gdb/testsuite/gdb.base/foll-exec.exp
  index 3e0653a1..61ee752f 100644
  --- a/gdb-8.3/gdb/testsuite/gdb.base/foll-exec.exp
  +++ b/gdb-8.3/gdb/testsuite/gdb.base/foll-exec.exp
  @@ -115,7 +115,10 @@ proc do_exec_tests {} {
  # We should stop in execd-program, at its first statement.
  #
  set execd_line [gdb_get_line_number "after-exec" $srcfile2]
  -   send_gdb "next\n"
  +# Clang will emit source locations for the parameter evaluation.
  +# Ideally we'd "next" until we saw 'execlp' again in the source display,
  +# then do one more.
  +   send_gdb "next 3\n"
  gdb_expect {
-re ".*xecuting new program: 
.*${testfile2}.*${srcfile2}:${execd_line}.*int  local_j = argc;.*$gdb_prompt $"\
{pass "step through execlp call"}
  @@ -263,7 +266,7 @@ proc do_exec_tests {} {
  # the newly-exec'd program, not after the remaining step-count
  # reaches zero.
  #
  -   send_gdb "next 2\n"
  +   send_gdb "next 3\n"
  gdb_expect {
-re ".*xecuting new program: 
.*${testfile2}.*${srcfile2}:${execd_line}.*int  local_j = argc;.*$gdb_prompt $"\
{pass "step through execl call"}

I think I might want to add one more lit test, because the LLVM suite didn't 
catch a thinko that was the root cause of that last prolog weirdness.  But this 
version is totally ready for review.


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.

I've run this through our copy of the GDB suite (8.3, not sure how old that 
is).  There are 10 differences, with and without the patch.

  FAIL: gdb.base/foll-exec.exp: step through execlp call
  FAIL: gdb.base/foll-exec.exp: step after execlp call
  FAIL: gdb.base/foll-exec.exp: print execd-program/global_i (after execlp)
  FAIL: gdb.base/foll-exec.exp: print execd-program/local_j (after execlp)
  FAIL: gdb.base/foll-exec.exp: print follow-exec/local_k (after execlp)

These 5 are all adjacent test points; what happens is that a "next" ends up 
stepping through parameter evaluation of a call to execl(), where the test 
expects it to execute the call.  Adding two more "next" commands should fix 
that.

  FAIL: gdb.base/call-ar-st.exp: check args of sum_array_print
  FAIL: gdb.base/funcargs.exp: continue to call2i
  FAIL: gdb.base/funcargs.exp: backtrace from call7k (pattern 1)

These three are all cases where a function parameter list is long enough to 
require parameters to be passed on the stack.  In that situation, prologue_end 
is being set *way* too early, and so breaking on the function will stop before 
the parameters have all been homed.  Garbage ensues.  Haven't gotten to the 
bottom of that yet.

The news is not all bad. These two changed to PASS:

  UNTESTED: gdb.base/break-caller-line.exp: target arch has an instruction 
after call as part of the caller line
  FAIL: gdb.python/python.exp: test find_pc_line with resume address


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

[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

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

See D92606  for a front-end patch to improve 
locations in the IR.
That, plus reapplying this patch, should help out GDB.  I haven't had a chance 
to run the suite myself with both patches applied and I'm basically off 
tomorrow, so if @dblaikie doesn't have a chance, I'll do that on Monday.


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 added a comment.

> Sometihng like this seems plausible to me:

Yes, I was playing with essentially that exact patch last night.  It has no 
effect on the final assembly on its own, but combined with my patch it does.

> (a more general fix (that would cover cases where the instruction really has 
> no location) might be to propagate locations from the first instruction in a 
> basic block with a location back up to the start of the basic block - I 
> forget if we've considered/tried that before)

We have, but that without flushing the map on every instruction, so it caught 
materialization instructions that didn't actually belong to that IR 
instruction.  The tactic would likely be more reasonable in conjunction with my 
patch.


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 added a comment.

@rnk is correct that the phi's source location is propagating to the xorl.  I 
modified the .ll file to put a different line number on the phi, and the xorl 
picked it up.  This is clearly on the front-end to get right.

If we want to get down to cases, there are other poor choices of source 
location in this tiny example.  The statement source is

  return a && b;

The 'a' is at column 10, the '&&' at 12, the 'b' at 15.
Now consider the load/test of 'a':

%0 = load i32, i32* %a.addr, align 4, !dbg !19
%tobool = icmp ne i32 %0, 0, !dbg !19
br i1 %tobool, label %land.rhs, label %land.end, !dbg !20
  ...
  !19 = !DILocation(line: 3, column: 10, scope: !12)
  !20 = !DILocation(line: 3, column: 12, scope: !12)

The load and icmp both use !19, the source location of 'a' in the expression; 
the br uses !20, the '&&'.  All seems good. Now consider the 'b' part:

%1 = load i32, i32* %b.addr, align 4, !dbg !21
%tobool1 = icmp ne i32 %1, 0, !dbg !20
br label %land.end
  ...
  !20 = !DILocation(line: 3, column: 12, scope: !12)
  !21 = !DILocation(line: 3, column: 15, scope: !12)

The load uses !21, 'b', but the icmp points to the '&&', and the br points 
nowhere.  This is at least inconsistent with how the LHS of the expression is 
handled, and I would argue it's actually wrong.

This gets reflected in the generated code:

.loc1 0 0 prologue_end  # gdb-fail.c:0:0
xorl%eax, %eax   because of the phi's line-0
  # kill: def $al killed $al killed $eax
.loc1 3 10  # gdb-fail.c:3:10
cmpl$0, -4(%rbp) load/compare of 'a' at 
column 10
movb%al, -9(%rbp)   # 1-byte Spill
.loc1 3 12 is_stmt 0# gdb-fail.c:3:12
je  .LBB1_2  the je attributed to the &&
  # %bb.1:# %land.rhs
cmpl$0, -8(%rbp) load/compare of 'b' at 
column 12 (??) not 15
setne   %al
movb%al, -9(%rbp)   # 1-byte Spill
  .LBB1_2:# %land.end
.loc1 0 12  # gdb-fail.c:0:12
movb-9(%rbp), %al   # 1-byte Reload

I'd also be happier if the reload was attributed to the same source location as 
the following instructions, but that's probably a whole 'nother can of worms.


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 added a comment.

Just for grins, change the `&&` to `||` and see what happens...
The xorl becomes a movb $1 (no surprise there).  But, that instruction no 
longer has line-0, instead it becomes part of the prologue.
This tells me that the xorl had an explicit line 0, while the 'movb $1' has no 
location (a subtle but important difference).

There are also curious differences between the CGExprScalar.cpp methods 
VisitBinLAnd() and VisitBinLOr(); the former is more attentive to line 
attributions than the latter, which seems likely to be an oversight dating back 
a decade or so.

Onward into the breach.


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 added a comment.

The constant materialization (xorl) obviously does not belong in the prologue; 
I think the instruction ordering with the patch is preferable.
But, clearly we'd rather not have it attributed to line 0, but the line it 
actually belongs to.  I'll start looking into this.  Offhand the reduced 
example appears to be not unreasonably large :-)

We do have a gdb suite of some form running on our downstream compiler, but 
this patch series hasn't made it there yet. When I put a new patch up I'll tag 
you @dblaikie and you can try it out on your bot.


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]

This revision was automatically updated to reflect the committed changes.
Closed by commit rGcf1c774d6ace: [FastISel] Flush local value map on ever 
instruction (authored by probinson).
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Changed prior to commit:
  https://reviews.llvm.org/D91734?vs=306479&id=307648#toc

Repository:
  rG LLVM Github Monorepo

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-elf-globals.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/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-NEXT:   DW_LLE_startx_length (0x0007, 0x0042): DW_OP_breg7 RSP-24
 ; CHECK-NEXT:   DW_LLE_end_of_list   ()
 ; CHECK:  [[D]]:
-; CHECK-NEXT:   DW_LLE_startx_length (0x0009, 0x000b): DW_OP_reg0 RAX
-; CHECK-NEXT:   DW_LLE_startx_length (0x000a, 0x002a): DW_OP_breg7 RSP-12
+; CHECK-NEXT:   DW_LLE_startx_length (0x0008, 0x000b): DW_OP_reg0 RAX
+; CHECK-NEXT:   DW_LLE_startx_length (0x0009, 0x002a): DW_OP_breg7 RSP-12
 ; CHECK-NEXT:   DW_LLE_end_of

[Lldb-commits] [PATCH] D71487: [LLDB] Fix address computation for inline function

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

In D71487#1791824 , @clayborg wrote:

> BTW: is used to be that both DW_AT_low_pc and DW_AT_high_pc would be set to 
> zero when a function was dead stripped. This was back when both the low and 
> high pc used DW_FORM_addr (a file address). But then DWARF changed such that 
> DW_AT_high_pc could be encoded as a data form: DW_FORM_data1, DW_FORM_data2, 
> DW_FORM_data4, or DW_FORM_data8. This is used to mean it is an offset from 
> the low PC. Seems the linkers now didn't have a relocation for the 
> DW_AT_high_pc so they couldn't zero it out. This is sad because we can end up 
> with many functions at address zero that didn't get linked, and if zero is a 
> valid address, then our DWARF contains a bunch of useless info that only 
> hides which function is the real function for address zero.


One solution, which we do in Sony, is to make the linker fix up undefined 
references to be -1 instead of 0 (at least, in the .debug_* sections).  That's 
more obviously an invalid address.  Doesn't help with existing objects in the 
wild but I'd like to keep that idea in the air as a forward evolutionary step.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71487



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

[Lldb-commits] [PATCH] D69309: Support template instantiation in the expression evaluator

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

In D69309#1787994 , @friss wrote:

> In D69309#1787409 , @labath wrote:
>
> > In D69309#1787297 , @jarin wrote:
> >
> > > In D69309#1752738 , @friss wrote:
> > >
> > > > Basically, today the debug info will describe an entity named 
> > > > "Foo". The accelerator tables all reference this name. So when 
> > > > Clang asks us if we know "Foo" (which is what happens when 
> > > > instantiating), we fail to find the right instantiations. The consensus 
> > > > of the above discussion was that we should change the debug info to 
> > > > have "Foo" as the name of any instantiation, with a child DIE 
> > > > describing the template arguments. Just doing this in the compiler 
> > > > causes test failures in LLDB, so there's some work to do in LLDB to 
> > > > support this.
> > >
> > >
> > > Frederic, you say that "doing this in the compiler causes test failures 
> > > in LLDB", which implies you have tried adding the template in the 
> > > compiler. Do you have that compiler patch lying around so that we could 
> > > have a look at what can be done on the lldb side?
> > >
> > > I agree that a good long term fix is to have "Foo" as an entity in DWARF, 
> > > although for backwards compatibility it might be better if the "Foo" 
> > > template just contained references to the instantiations rather than 
> > > having them as children.
> >
> >
> > I am afraid you're overestimating the scope of that idea. I *think* that 
> > Fred was referring to simply changing the string that gets put into the 
> > DW_AT_name field of the /instantation/ (and, by extension, the accelerator 
> > table). The debug info would still describe instantiations only.
>
>
> Just confirming that this is indeed what I meant. I don't have the patch 
> handy anymore (it was extremely small, about 5 lines IIRC).


FWIW, the Sony debugger throws away the `` part of the DW_AT_name and 
reconstructs it from the template_parameter children.  The presence of typedefs 
and/or enums can make the `` text inconsistent, especially across enums.  
Our debugger reconstructs the type-parameters because it's more consistent that 
way.


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

https://reviews.llvm.org/D69309



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

[Lldb-commits] [PATCH] D49466: Initial implementation of -fmacro-prefix-map and -ffile-prefix-map

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

In D49466#1761156 , @MaskRay wrote:

> The ugly path separator pattern `{{(/|)}}` appears in 60+ tests. Can we 
> teach clang and other tools to
>
> 1. accept both `/` and `\` input


In general they do, AFAIK, although it's not feasible in cases where `/` is the 
character that introduces an option, which is common on standard Windows 
utilities.

> 2. but only output `/`
> 
>   on Windows?

This is often actually incorrect for Windows.

> We can probably remove llvm::sys::path::Style::{windows,posix,native} from 
> include/llvm/Support/Path.h and only keep the posix form.

It's highly unlikely that will be correct for all cases, and certainly will not 
match users' expectations.  Debug info, for example, will want normal Windows 
syntax file paths with `\`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D49466



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

[Lldb-commits] [PATCH] D68270: DWARFDebugLoc: Add a function to get the address range of an entry

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

Do we care whether llvm-dwarfdump's output bears any similarities to the output 
from GNU readelf or objdump?  There has been a push lately to get the LLVM 
"binutils" to behave more like GNU's, although AFAIK it hasn't gotten to the 
DWARF dumping part.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D68270



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

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

Pick whatever mechanism you like, we should debate it in that patch not here.  
:-)


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63591



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

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

Ah, hadn't considered statefulness.  But if you layer another class on top of 
DataExtractor to handle the error flag, it would have to be replicating all the 
offset-is-valid checks, because of course DataExtractor itself doesn't return 
errors.

I have a couple more ideas to toss out there...

- A DataExtractorBase class that returns Optional, and then 
DataExtractor layers on top and converts None to zero, which preserves the 
non-statefulness as well as the current API. This adds some runtime overhead, 
not sure how much.
- Or, a template DataExtractorBase that takes an error-handling class as a 
parameter (sort of like how STL containers take an allocator) and a 
DataExtractor specialization uses a no-op error-handling class. Should avoid 
runtime overhead at the cost of template cruft.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63591



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

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

Removing that llvm_unreachable is fine, in that case.
The idea for error handling for DataExtractor sounds reasonable, looks like 
adding an error flag wouldn't even increase the size.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63591



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

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

[Paul Robinson via Phabricator via lldb-commits]

probinson added inline comments.



Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:220
   }
-  return LL;
+  llvm_unreachable("Exit from function inside while loop!");
 }

dblaikie wrote:
> Given the loop condition is "while (true)" this unreachable seems a bit 
> unnecessary (& the function has non-void return, so if there was a path that 
> got through the loop I imagine the compiler would warn us about that?)
> 
> Or is this working around a compiler that warns here despite the lack of any 
> path out of the loop?
I have had to add llvm_unreachable before in this kind of situation, IIRC, 
which is why I suggested it.  Might not be necessary, if all 3 supported 
toolchains are smart enough nowadays.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63591



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

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

[Paul Robinson via Phabricator via lldb-commits]

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

LGTM but give the West Coast folks a chance to look at it.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63591



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

[Lldb-commits] [PATCH] D63591: DWARFDebugLoc: Make parsing and error reporting more robust

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

Looks pretty good, and thanks especially for the error-case tests!
I'll give other folks a chance to chime in if they want to.




Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:101
+if (!Data.isValidOffsetForDataOfSize(*Offset, 2 * Data.getAddressSize()))
+  return createError("location list overflows the debug_loc section");
 

This identical createError call occurs many times, maybe add a 
createLocListOverflowError() helper?



Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:115
 
-if (!Data.isValidOffsetForDataOfSize(*Offset, 2)) {
-  WithColor::error() << "location list overflows the debug_loc section.\n";
-  return None;
-}
+if (!Data.isValidOffsetForDataOfSize(*Offset, 2))
+  return createError("location list overflows the debug_loc section");

You could do `SavedOffset = *Offset;` here, and then add a `SavedOffset == 
*Offset` check to the next one.  There's no harm to calling a `get*` function 
with an invalid offset.



Comment at: lib/DebugInfo/DWARF/DWARFDebugLoc.cpp:218
   }
-  return LL;
 }
 

Maybe put an llvm_unreachable here.



Comment at: test/DebugInfo/X86/dwarfdump-debug-loc-error-cases.s:1
+# RUN: llvm-mc %s -filetype obj -triple x86_64-pc-linux --defsym CASE1=0 -o 
%t.o
+# RUN: llvm-dwarfdump -debug-loc %t.o 2>&1 | FileCheck %s 
--check-prefix=CONSUME

I was not aware of `--defsym` that looks incredibly useful!

In a test that generates multiple .o files I prefer to give each one a unique 
name, e.g. `%t0.o` and `%t1.o` etc.  It can make it easier to debug a broken 
test.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D63591



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

[Lldb-commits] [PATCH] D62634: Improve DWARF parsing and accessing by 1% to 2%

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

In D62634#1527575 , @dblaikie wrote:

> This is intentional behavior in clang (controllable under the 
> -f[no-]split-dwarf-inlining flag, and modified by the 
> -f[no-]debug-info-for-profiling flag).
>
> This extra debug info is used for online symbolication (in the absence of 
> .dwo files) - such as for the sanitizers (accurate symbolication is necessary 
> for correctness in msan, due to msan's necessary use of blacklisting to avoid 
> certain false positives) and some forms of sample based profiling collection.
>
> In the default mode, clang includes, as you noted, just the subprograms that 
> have inlined subroutines in them in this split-dwarf-inlining data.
>  In -fdebug-info-for-profiling all subprograms are described in the skeleton 
> CU with some minimal attributes (they don't need parameters, local 
> variables/scopes, etc) necessary to do certain profile things I don't know 
> lots about.


I think we have to tolerate the msan part.  However, the profile feedback 
workflow could (should) surely be taught to read a .dwo file.  The point of 
-fdebug-info-for-profiling was so you don't have to emit limited/full debug 
info in the .o file just to make profiling work; but if you're using split 
DWARF then you're doing limited/full anyway, and the feedback loop happens in 
the context of a build environment so the .dwo can be asserted to be available. 
 So in split DWARF mode, we should not be emitting debug-info-for-profiling 
into the skeleton.

> Chances are it's probably best for a debugger to ignore these entries.

This stuff is going to show up in the wild, so yeah.  You know you're looking 
at a skeleton, so don't bother looking at any children.


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

https://reviews.llvm.org/D62634



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

[Lldb-commits] [PATCH] D62302: DWARF: Fix address range support in mixed 4+5 scenario

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

Drive-by: For the "dead code" did you check whether gcc emits 
DW_AT_start_scope?  LLDB should support more than just what Clang emits.


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

https://reviews.llvm.org/D62302



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

[Lldb-commits] [PATCH] D61752: Re-enable a test for non-Windows

[Paul Robinson via Phabricator via lldb-commits]

probinson abandoned this revision.
probinson added a comment.

@stella.stamenova  fixed this using system-windows instead.


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D61752



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

[Lldb-commits] [PATCH] D61752: Re-enable a test for non-Windows

[Paul Robinson via Phabricator via lldb-commits]

probinson created this revision.
probinson added reviewers: stella.stamenova, labath.
Herald added a project: LLDB.
Herald added a subscriber: lldb-commits.

Required teaching lit.cfg.py to distinguish Windows the same way other projects 
do.


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D61752

Files:
  lldb/lit/Breakpoint/case-sensitive.test
  lldb/lit/lit.cfg.py


Index: lldb/lit/lit.cfg.py
===
--- lldb/lit/lit.cfg.py
+++ lldb/lit/lit.cfg.py
@@ -96,3 +96,7 @@
 config.available_features.add('native-cpu-%s' % x)
 else:
 lit_config.warning("lit-cpuid failed: %s" % err)
+
+# Allow distinguishing Windows host from others.
+if sys.platform.startswith('win') or sys.platform.startswith('cygwin'):
+config.available_features.add('windows')
Index: lldb/lit/Breakpoint/case-sensitive.test
===
--- lldb/lit/Breakpoint/case-sensitive.test
+++ lldb/lit/Breakpoint/case-sensitive.test
@@ -1,4 +1,4 @@
-# REQUIRES: nowindows
+# UNSUPPORTED: windows
 #
 # RUN: %build %p/Inputs/case-sensitive.c --nodefaultlib -o %t
 # RUN: lldb-test breakpoints %t %s | FileCheck %s


Index: lldb/lit/lit.cfg.py
===
--- lldb/lit/lit.cfg.py
+++ lldb/lit/lit.cfg.py
@@ -96,3 +96,7 @@
 config.available_features.add('native-cpu-%s' % x)
 else:
 lit_config.warning("lit-cpuid failed: %s" % err)
+
+# Allow distinguishing Windows host from others.
+if sys.platform.startswith('win') or sys.platform.startswith('cygwin'):
+config.available_features.add('windows')
Index: lldb/lit/Breakpoint/case-sensitive.test
===
--- lldb/lit/Breakpoint/case-sensitive.test
+++ lldb/lit/Breakpoint/case-sensitive.test
@@ -1,4 +1,4 @@
-# REQUIRES: nowindows
+# UNSUPPORTED: windows
 #
 # RUN: %build %p/Inputs/case-sensitive.c --nodefaultlib -o %t
 # RUN: lldb-test breakpoints %t %s | FileCheck %s
___
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D61611: [JITLoaderGDB] Set eTypeJIT for objects read from JIT descriptors

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

In D61611#1496865 , @stella.stamenova 
wrote:

> In D61611#1496838 , @probinson wrote:
>
> > @stella.stamenova I'm not familiar with any lit feature that gives a 
> > special meaning to the prefix "no".  The opposite of "REQUIRES: windows" is 
> > not "REQUIRES: nowindows" but "UNSUPPORTED: windows" AFAIK.
> >  This part of the discussion should probably be taken to llvm-dev, though.
> >
> > FTR I don't see that lldb's lit.cfg.py sets any features based on host OS, 
> > so even "UNSUPPORTED: windows" probably does not work currently.
>
>
> @probinson LLDB's lit configuration derives from the lit configuration in 
> LLVM. The most important file you are looking for is:
>
> `llvm\utils\lit\lit\llvm\config.py`
>
> You can see there that we add various platform to the list of available 
> features including system-windows, so XFAIL: system-windows, UNSUPPORTED: 
> system-windows, etc. all work.
>
> This is also where binary_features are set, such as 'zlib/nozlib', 
> 'asan/not_asan' etc. The prefix varies for historical reasons, but the 
> paradigm has existed for a long time. If we wanted to support 
> nosystem-windows, we would add it here.


Thanks @stella.stamenova I did find that.  But I agree with Pavel, we are 
better off not having "nofoo" or "not_foo" features, because we can get the 
same effect using UNSUPPORTED: and it can mislead people into thinking the 
no/not_ prefix applies generally, and things like "REQUIRES: nowindows" will 
unexpectedly disable a test everywhere.

I am going to propose eliminating the negative keywords on llvm-dev.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61611



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

[Lldb-commits] [PATCH] D61611: [JITLoaderGDB] Set eTypeJIT for objects read from JIT descriptors

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

@stella.stamenova I'm not familiar with any lit feature that gives a special 
meaning to the prefix "no".  The opposite of "REQUIRES: windows" is not 
"REQUIRES: nowindows" but "UNSUPPORTED: windows" AFAIK.
This part of the discussion should probably be taken to llvm-dev, though.

FTR I don't see that lldb's lit.cfg.py sets any features based on host OS, so 
even "UNSUPPORTED: windows" probably does not work currently.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D61611



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

[Lldb-commits] [PATCH] D61611: [JITLoaderGDB] Set eTypeJIT for objects read from JIT descriptors

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

In D61611#1496580 , @sgraenitz wrote:

> @stella.stamenova Can you have a look at the lit test please? It works on 
> macOS and Linux, but I didn't test Windows. Should I add something like `# 
> REQUIRES: nowindows` or is it fine like this?


Sorry for the drive-by... what is this `REQUIRES: nowindows`?  I don't see 
where lit generates this property.  I grepped all of llvm-project.git and I see 
it used in two tests, but not where it's produced. Which suggests that those 
tests don't actually run *anywhere*.
I think maybe you want `UNSUPPORTED: system-windows` instead?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61611



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

[Lldb-commits] [PATCH] D59562: [SymbolFileDWARF] Introduce DWARFContext

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

I remember LLVM's DWARFContext being irritating to deal with when I was doing 
the DWARF 5 stuff.  The exact details have been swapped out of my memory, but I 
suspect it had something to do with needing to know whether I was in DWO mode 
or normal mode in order to snag the right section for some sort of 
cross-section lookup.  This is because DWARFContext is the dumping ground for 
all the sections, regardless of what kind of file we're looking at, and it's up 
to the user of its interfaces to know what mode it's in.
I will admit that having one dumping ground is convenient in some ways, 
especially when we were looking at object files that contained a mix of both 
normal and .dwo sections, but IIRC we don't emit mixed-mode object files 
anymore.

Not objecting to this patch, but it was the most convenient place to raise this 
particular grump, and something to think about as you progress.


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

https://reviews.llvm.org/D59562



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

[Lldb-commits] [PATCH] D59235: Remove Support for DWARF64

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

In D59235#1425443 , @zturner wrote:

> In D59235#1425436 , @clayborg wrote:
>
> > My main concern with the LLVM DWARF parser is all of the asserts in the 
> > code. If you attempt to use a DWARFDIE without first checking it for 
> > validity, it will crash on you instead of returning a good error or default 
> > value. That makes me really nervous as we shouldn't just crash the 
> > debugger. The switching over won't be too hard, just the fallout from the 
> > LLDB versions of the class that do error checking and return good 
> > error/default values and LLVM being very strict.
>
>
> Sure, I'm prepared to deal all that appropriately.  I don't plan to regress 
> LLDB's stability in the process.
>
> That's why for now I'm just doing very small preliminary steps to get the two 
> interfaces to be closer to each other and simplify the problem space.  We can 
> worry about the asserts and all of that when we actually start moving pieces 
> of LLDB to use LLVM's classes (which isn't in this patch).


A long term plan of moving LLVM's parser away from asserts and toward error 
reporting on bad input would also make the binutils that try to read DWARF more 
robust and useful for trying to diagnose bad object files.  I'm all for it.


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

https://reviews.llvm.org/D59235



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

[Lldb-commits] [PATCH] D51935: [LLDB] - Improve reporting source lines and variables (improved DWARF5 support).

[Paul Robinson via Phabricator via lldb-commits]

probinson added inline comments.



Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugLine.cpp:442
+ReadDescriptors(debug_line_data, offset_ptr);
+uint8_t dirCount = debug_line_data.GetU8(offset_ptr);
+for (int i = 0; i < dirCount; ++i) {

clayborg wrote:
> We might verify that this is indeed correct by looking at compiler sources. 
> Seems weird to limit dirs to 255
The directory count is a ULEB not a ubyte.


https://reviews.llvm.org/D51935



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

[Lldb-commits] [PATCH] D50525: [WIP] Move lldb-mi interpreter tests to LIT

[Paul Robinson via Phabricator via lldb-commits]

probinson added inline comments.



Comment at: lit/tools/lldb-mi/interpreter/cli-support/target-list.test:5
+# We should hardcode executable name since at the moment of running of
+# lldb-mi the name must be known.
+# RUN: %cxx -o a.exe %p/inputs/main.cpp -g

aprantl wrote:
> That's totally fine, we just need to choose a name that works on all 
> platforms, and `a.exe` does.
Not `%t/a.exe` ?  I thought CWD is sometimes not writeable (or might be the 
working copy, and we don't want to leave trash there).


Repository:
  rLLDB LLDB

https://reviews.llvm.org/D50525



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

[Lldb-commits] [PATCH] D50027: Added initial unit test for LLDB's Stream class.

[Paul Robinson via Phabricator via lldb-commits]

probinson added inline comments.



Comment at: unittests/Utility/StreamTest.cpp:38-41
+TEST_F(StreamTest, ChangingByteOrder) {
+  s.SetByteOrder(lldb::eByteOrderPDP);
+  EXPECT_EQ(lldb::eByteOrderPDP, s.GetByteOrder());
+}

teemperor wrote:
> labath wrote:
> >  I've been wondering for a while whether we shouldn't just remove 
> > PDP byte order support. Most of our code doesn't really support it, and 
> > neither does llvm's, so this is kind of a prerequisite for switching to 
> > llvm streams. 
> I support this notion.  think most of LLDB's algorithms do not respect PDP 
> ordering.
PDP ordering?  As in, PDP-11?  I could imagine GDB had to support it many long 
ages ago, but AFAIK no such machine has been produced this century.


https://reviews.llvm.org/D50027



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

[Lldb-commits] [PATCH] D49192: [FileCheck] Add -allow-deprecated-dag-overlap to failing lldb tests

[Paul Robinson via Phabricator via lldb-commits]

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

LGTM


https://reviews.llvm.org/D49192



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

[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

Go for it.  It sounds like an abstraction that hides the multiple-section 
nature of the beast will particularly help LLDB; on the LLVM side the dumper 
will want to remain a bit more cognizant of the underlying sections.  But we 
can surely leverage ideas from each other, which will ultimately help with 
converging on the Grand Unified Parser.


https://reviews.llvm.org/D32167



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

[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

I haven't given Pavel's comments as much thought as they deserve, but here are 
some of mine.

It's a worthwhile point that a class ought to represent some useful and 
reasonably well defined abstraction.  DataExtractor has one, and 
DWARFDataExtractor builds on that (at least in the LLVM parser) by handling 
relocations; the most reasonable way to handle multiple buffers would be with 
multiple extractors, building on top of what's there rather than hacking the 
insides.

One aspect I am looking at is that the LLVM parser needs to handle .o files, 
which can have multiple .debug_types (in v4) or .debug_info (in v5) sections, 
because the units are broken up into COMDATs.  So, the dumper needs to be able 
to handle an arbitrary number of either kind of section, where LLDB needs at 
most one of each section.  One way of dealing with all this is to have a vector 
or deque of sections, each section has its DWARFDataExtractor and knows its own 
"base offset" which would be the sum of the sizes of the preceding sections.  I 
don't know how awkward it might be to turn this into a meta-extractor that hid 
details from consumers, but then I think it probably would not be all that hard 
to have consumers be aware that there can be multiple lumps of DWARF and get 
the offsets right on their own.


https://reviews.llvm.org/D32167



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

[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

I can't coment on the xcode and python parts.  The C++ all looks plausible.
I think the "sliding" trick will be trickier in the LLVM parser, as it has to 
handle .o files (which might have multiple instances of .debug_types in v4, and 
multiple instances of .debug_info in v5) but is conceptually nice.  The sliding 
should be kept to DWARFDataExtractor and not pushed down into DataExtractor, 
IMO, because the problem is (I think) pretty specific to DWARF.


https://reviews.llvm.org/D32167



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

[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

So, I've been attacking the type-units-in-.debug_info problem in LLVM, and what 
I've done (so far) in the LLVM parser is to factor out a DWARFUnitHeader class, 
and made DWARFUnit derive from that.  This makes it feasible to parse a unit 
header before deciding what kind of unit we are looking at, which is necessary 
when .debug_info contains both type and compile units.  I haven't posted that 
patch yet as it's an NFC prep for more work getting to the point of actually 
allowing mix-n-match units in .debug_info sections.  But I'll post that patch 
today so people can at least look at it.

Mainly Greg and I should not take diverging approaches when we can converge 
instead.


https://reviews.llvm.org/D32167



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

[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

I am looking at making the LLVM parser handle type units across v4/v5, and the 
trick of pretending .debug_types is pasted onto the end of .debug_info seems 
like a pretty convenient fiction.  It unifies DIE handling, and still allows 
the dumper to distinguish which bits are in which section, which is 
traditionally how the dumper UI thinks about DWARF.  So as long as you and Jan 
are on the same page there, I'm happy to follow suit.




Comment at: source/Plugins/SymbolFile/DWARF/DIERef.cpp:53
+  // and DIE for the type signature. When a type is referred to by a
+  // DW_FORM_ref_sig8 form, the real information for the type in
+  // contained in a DW_TAG_type_unit.

... the type is


https://reviews.llvm.org/D32167



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

[Lldb-commits] [PATCH] D32167: Add support for type units (.debug_types) to LLDB in a way that is compatible with DWARF 5

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

In https://reviews.llvm.org/D32167#1020032, @labath wrote:

> Running the entire dotest test suite in -fdebug-types-section is certainly a 
> good way to catch problems, but it's not the way to write regression tests.


Is there testing in place that is more serious/thorough than the normal 
regression testing?  It might be reasonable to do the full cartesian set at a 
slower pace.  For example LLVM has the concept of "expensive checks" and there 
are a small number of bots out there that enable them.  But it's not something 
anybody runs normally.


https://reviews.llvm.org/D32167



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

[Lldb-commits] [PATCH] D39896: Remove last Host usage from ArchSpec

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

Drive by comment:

In https://reviews.llvm.org/D39896#94, @jingham wrote:

> You're right, the Triple stuff is in ADT!  I was using it as an example of 
> something you clearly wouldn't do so I didn't actually check to see if it was 
> done.  That's pretty funny.
>
> I thought that ADT stood for Abstract Data Types - though I actually I don't 
> remember where I got that impression...  Having Triple there does seem 
> confusing.  That is a "how we specify Targets type thing", not a fancy data 
> type type thing...  I would never think to look for that facility alongside 
> StringRef & DenseMap etc.


Triple.h is in ADT; Triple.cpp is in Support.  I agree it belongs in Support, 
and I'm not sure why it's schizo like that.


https://reviews.llvm.org/D39896



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

[Lldb-commits] [PATCH] D39215: Default to using in-tree clang for building test executables

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

Has this gone in?  I'm wondering because I starting playing with the monorepo, 
ran cmake with -DLLDB_TEST_COMPILER=$PWD/bin/clang, and today's test failure 
seems to be trying to build the test program with the system compiler (gcc) 
rather than my copy of clang.  But it looks like you're deprecating 
LLDB_TEST_COMPILER?


https://reviews.llvm.org/D39215



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

[Lldb-commits] [PATCH] D35734: Don't allow LLDB to try and parse .debug_types

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

In https://reviews.llvm.org/D35734#818778, @tberghammer wrote:

> This section have been already removed from Dwarf5 so I agree that we 
> shouldn't spend too much time adding support for it.


Compilers wanting to use type units and DWARF 4 should be emitting them in the 
.debug_types section.  DWARF 5 kept the concept but moved them back into the 
.debug_info section.  Supporting type units in general seems like a good idea, 
and the only question is where they live and what the exact details of the 
header look like.


Repository:
  rL LLVM

https://reviews.llvm.org/D35734



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

[Lldb-commits] [PATCH] D32787: Fix build error: no viable conversion from returned value of type 'int' to function return type 'sigset_t' (aka '__sigset_t')

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

In https://reviews.llvm.org/D32787#745205, @labath wrote:

> In https://reviews.llvm.org/D32787#745139, @probinson wrote:
>
> > In https://reviews.llvm.org/D32787#745099, @labath wrote:
> >
> > > Could it be that you just have stale cmake cache?
> >
> >
> > That could easily be true.  Rerunning cmake didn't fix it; short of 
> > deleting the entire build directory, is there a way to refresh the cache?
>
>
> Opening the CMakeCache.txt, and deleting the HAVE_PPOLL line should 
> re-trigger the detection. But if that doesn't fix it, I'd certainly recommend 
> nuking the build directory before looking for problems elsewhere.


Yes, that worked.  Which leaves the original source-text problem, of course, 
but at least I'm not having to maintain a local patch anymore.
Thanks!


Repository:
  rL LLVM

https://reviews.llvm.org/D32787



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

[Lldb-commits] [PATCH] D32787: Fix build error: no viable conversion from returned value of type 'int' to function return type 'sigset_t' (aka '__sigset_t')

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

In https://reviews.llvm.org/D32787#745099, @labath wrote:

> Could it be that you just have stale cmake cache?


That could easily be true.  Rerunning cmake didn't fix it; short of deleting 
the entire build directory, is there a way to refresh the cache?


Repository:
  rL LLVM

https://reviews.llvm.org/D32787



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

[Lldb-commits] [PATCH] D32787: Fix build error: no viable conversion from returned value of type 'int' to function return type 'sigset_t' (aka '__sigset_t')

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

Looking at CMakeError.log, the test program does `#include ` but does 
not `#define _GNU_SOURCE`.  The define has to be there for your example to 
compile on my Ubuntu.


Repository:
  rL LLVM

https://reviews.llvm.org/D32787



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

[Lldb-commits] [PATCH] D27282: Accommodate line-0 records in a test

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

Thanks for helping to decipher the test result!


Repository:
  rL LLVM

https://reviews.llvm.org/D27282



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

[Lldb-commits] [PATCH] D27282: Accommodate line-0 records in a test

[Paul Robinson via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL288282: PR31214: Make a test tolerate "line 0" when stepping 
by instruction. (authored by probinson).

Changed prior to commit:
  https://reviews.llvm.org/D27282?vs=79813&id=79824#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D27282

Files:
  
lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/exit_during_step/TestExitDuringStep.py


Index: 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/exit_during_step/TestExitDuringStep.py
===
--- 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/exit_during_step/TestExitDuringStep.py
+++ 
lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/exit_during_step/TestExitDuringStep.py
@@ -23,32 +23,35 @@
 self.build(dictionary=self.getBuildFlags())
 self.exit_during_step_base(
 "thread step-inst -m all-threads",
-'stop reason = instruction step')
+'stop reason = instruction step',
+True)
 
 @skipIfFreeBSD  # llvm.org/pr21411: test is hanging
 def test_step_over(self):
 """Test thread exit during step-over handling."""
 self.build(dictionary=self.getBuildFlags())
 self.exit_during_step_base(
 "thread step-over -m all-threads",
-'stop reason = step over')
+'stop reason = step over',
+False)
 
 @skipIfFreeBSD  # llvm.org/pr21411: test is hanging
 def test_step_in(self):
 """Test thread exit during step-in handling."""
 self.build(dictionary=self.getBuildFlags())
 self.exit_during_step_base(
 "thread step-in -m all-threads",
-'stop reason = step in')
+'stop reason = step in',
+False)
 
 def setUp(self):
 # Call super's setUp().
 TestBase.setUp(self)
 # Find the line numbers to break and continue.
 self.breakpoint = line_number('main.cpp', '// Set breakpoint here')
 self.continuepoint = line_number('main.cpp', '// Continue from here')
 
-def exit_during_step_base(self, step_cmd, step_stop_reason):
+def exit_during_step_base(self, step_cmd, step_stop_reason, 
by_instruction):
 """Test thread exit during step handling."""
 exe = os.path.join(os.getcwd(), "a.out")
 self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
@@ -111,6 +114,9 @@
 
 current_line = frame.GetLineEntry().GetLine()
 
+if by_instruction and current_line == 0:
+continue
+
 self.assertGreaterEqual(
 current_line,
 self.breakpoint,


Index: lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/exit_during_step/TestExitDuringStep.py
===
--- lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/exit_during_step/TestExitDuringStep.py
+++ lldb/trunk/packages/Python/lldbsuite/test/functionalities/thread/exit_during_step/TestExitDuringStep.py
@@ -23,32 +23,35 @@
 self.build(dictionary=self.getBuildFlags())
 self.exit_during_step_base(
 "thread step-inst -m all-threads",
-'stop reason = instruction step')
+'stop reason = instruction step',
+True)
 
 @skipIfFreeBSD  # llvm.org/pr21411: test is hanging
 def test_step_over(self):
 """Test thread exit during step-over handling."""
 self.build(dictionary=self.getBuildFlags())
 self.exit_during_step_base(
 "thread step-over -m all-threads",
-'stop reason = step over')
+'stop reason = step over',
+False)
 
 @skipIfFreeBSD  # llvm.org/pr21411: test is hanging
 def test_step_in(self):
 """Test thread exit during step-in handling."""
 self.build(dictionary=self.getBuildFlags())
 self.exit_during_step_base(
 "thread step-in -m all-threads",
-'stop reason = step in')
+'stop reason = step in',
+False)
 
 def setUp(self):
 # Call super's setUp().
 TestBase.setUp(self)
 # Find the line numbers to break and continue.
 self.breakpoint = line_number('main.cpp', '// Set breakpoint here')
 self.continuepoint = line_number('main.cpp', '// Continue from here')
 
-def exit_during_step_base(self, step_cmd, step_stop_reason):
+def exit_during_step_base(self, step_cmd, step_stop_reason, by_instruction):
 """Test thread exit during step handling."""
 exe = os.path.join(os.getcwd(), "a.out")
 self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
@@ -111,6 +114,9 @@
 
 current_line = frame.GetLineEntry().GetLine()
 
+if by_instruction and current_line == 0:
+continue
+
 self.asser

[Lldb-commits] [PATCH] D27282: Accommodate line-0 records in a test

[Paul Robinson via Phabricator via lldb-commits]

probinson added a comment.

Forgot to mention this is for PR31214.


https://reviews.llvm.org/D27282



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

[Lldb-commits] [PATCH] D27282: Accommodate line-0 records in a test

[Paul Robinson via Phabricator via lldb-commits]

probinson created this revision.
probinson added a reviewer: jingham.
probinson added a subscriber: LLDB.

LLVM is about to start emitting "line 0" records more often in the DWARF line 
table.
One test tripped over this; made the test accommodate the new records.


https://reviews.llvm.org/D27282

Files:
  
packages/Python/lldbsuite/test/functionalities/thread/exit_during_step/TestExitDuringStep.py


Index: 
packages/Python/lldbsuite/test/functionalities/thread/exit_during_step/TestExitDuringStep.py
===
--- 
packages/Python/lldbsuite/test/functionalities/thread/exit_during_step/TestExitDuringStep.py
+++ 
packages/Python/lldbsuite/test/functionalities/thread/exit_during_step/TestExitDuringStep.py
@@ -23,32 +23,35 @@
 self.build(dictionary=self.getBuildFlags())
 self.exit_during_step_base(
 "thread step-inst -m all-threads",
-'stop reason = instruction step')
+'stop reason = instruction step',
+True)
 
 @skipIfFreeBSD  # llvm.org/pr21411: test is hanging
 def test_step_over(self):
 """Test thread exit during step-over handling."""
 self.build(dictionary=self.getBuildFlags())
 self.exit_during_step_base(
 "thread step-over -m all-threads",
-'stop reason = step over')
+'stop reason = step over',
+False)
 
 @skipIfFreeBSD  # llvm.org/pr21411: test is hanging
 def test_step_in(self):
 """Test thread exit during step-in handling."""
 self.build(dictionary=self.getBuildFlags())
 self.exit_during_step_base(
 "thread step-in -m all-threads",
-'stop reason = step in')
+'stop reason = step in',
+False)
 
 def setUp(self):
 # Call super's setUp().
 TestBase.setUp(self)
 # Find the line numbers to break and continue.
 self.breakpoint = line_number('main.cpp', '// Set breakpoint here')
 self.continuepoint = line_number('main.cpp', '// Continue from here')
 
-def exit_during_step_base(self, step_cmd, step_stop_reason):
+def exit_during_step_base(self, step_cmd, step_stop_reason, 
by_instruction):
 """Test thread exit during step handling."""
 exe = os.path.join(os.getcwd(), "a.out")
 self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
@@ -111,6 +114,9 @@
 
 current_line = frame.GetLineEntry().GetLine()
 
+if by_instruction and current_line == 0:
+continue
+
 self.assertGreaterEqual(
 current_line,
 self.breakpoint,


Index: packages/Python/lldbsuite/test/functionalities/thread/exit_during_step/TestExitDuringStep.py
===
--- packages/Python/lldbsuite/test/functionalities/thread/exit_during_step/TestExitDuringStep.py
+++ packages/Python/lldbsuite/test/functionalities/thread/exit_during_step/TestExitDuringStep.py
@@ -23,32 +23,35 @@
 self.build(dictionary=self.getBuildFlags())
 self.exit_during_step_base(
 "thread step-inst -m all-threads",
-'stop reason = instruction step')
+'stop reason = instruction step',
+True)
 
 @skipIfFreeBSD  # llvm.org/pr21411: test is hanging
 def test_step_over(self):
 """Test thread exit during step-over handling."""
 self.build(dictionary=self.getBuildFlags())
 self.exit_during_step_base(
 "thread step-over -m all-threads",
-'stop reason = step over')
+'stop reason = step over',
+False)
 
 @skipIfFreeBSD  # llvm.org/pr21411: test is hanging
 def test_step_in(self):
 """Test thread exit during step-in handling."""
 self.build(dictionary=self.getBuildFlags())
 self.exit_during_step_base(
 "thread step-in -m all-threads",
-'stop reason = step in')
+'stop reason = step in',
+False)
 
 def setUp(self):
 # Call super's setUp().
 TestBase.setUp(self)
 # Find the line numbers to break and continue.
 self.breakpoint = line_number('main.cpp', '// Set breakpoint here')
 self.continuepoint = line_number('main.cpp', '// Continue from here')
 
-def exit_during_step_base(self, step_cmd, step_stop_reason):
+def exit_during_step_base(self, step_cmd, step_stop_reason, by_instruction):
 """Test thread exit during step handling."""
 exe = os.path.join(os.getcwd(), "a.out")
 self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
@@ -111,6 +114,9 @@
 
 current_line = frame.GetLineEntry().GetLine()
 
+if by_instruction and current_line == 0:
+continue
+
 self.assertGreaterEqual(
 current_line,
 self.breakpoint,
_