[Lldb-commits] [PATCH] D142552: [lldb] Make GetDIENamesAndRanges() allow 0-valued decl and call lines

[David Stenberg via Phabricator via lldb-commits]

dstenb created this revision.
dstenb added reviewers: jasonmolenda, clayborg, aeubanks, ayermolo.
dstenb added a project: LLDB.
Herald added a subscriber: JDevlieghere.
Herald added a reviewer: shafik.
Herald added a project: All.
dstenb requested review of this revision.
Herald added a reviewer: jdoerfert.
Herald added subscribers: lldb-commits, sstefan1.

In an upcoming patch Clang is proposed to be changed to emit
line locations that are inlined at line 0. This clashed with the behavior of
GetDIENamesAndRanges() which used 0 as a default value to determine if
file, line or column numbers had been set. Users of that function then
checked for any non-0 values when setting up the call site:

  if (call_file != 0 || call_line != 0 || call_column != 0)
[...]

which did not work with the Clang change since all three values then
could be 0.

This changes the function to use std::optional to catch non-set values
instead.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142552

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/test/Shell/SymbolFile/DWARF/Inputs/inlined-file0-line0-col0.yaml
  lldb/test/Shell/SymbolFile/DWARF/inlined-file0-line0-col0.test

Index: lldb/test/Shell/SymbolFile/DWARF/inlined-file0-line0-col0.test
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/inlined-file0-line0-col0.test
@@ -0,0 +1,34 @@
+# RUN: yaml2obj %S/Inputs/inlined-file0-line0-col0.yaml -o %t
+# RUN: %lldb %t -s %s -o exit | FileCheck %s
+
+#  1 void abort(void);
+#  2 int g1 = 4, g2 = 6;
+#  3
+#  4 inline __attribute__((always_inline)) void bar(int q) {
+#  5   if (q > 5)
+#  6 abort();
+#  7 }
+#  8
+#  9 inline __attribute__((always_inline)) void foo(int q) {
+# 10   bar(q);
+# 11 }
+# 12
+# 13 int main() {
+# 14   foo(g1);
+# 15   foo(g2);
+# 16   return 0;
+# 17 }
+
+# The input object file contains a single abort invocation for the two inlined
+# instances of foo() in main() at line 0. As the file, line and column numbers
+# are all 0, file and line number information would be missing for foo and main
+# in the lookup information.
+#
+# A line number 0 is not printed for main in this case, but the same holds
+# for a non-inlined location with line number 0.
+
+# CHECK: Summary: inlined-file0-line0-col0.test.tmp`main + 30 [inlined] bar + 4 at inlined-file0-line0-col0.c:6:5
+# CHECK-NEXT: inlined-file0-line0-col0.test.tmp`main + 26 [inlined] foo at inlined-file0-line0-col0.c:10:3
+# CHECK-NEXT: inlined-file0-line0-col0.test.tmp`main + 26 at inlined-file0-line0-col0.c
+
+image lookup -a 0x1e
Index: lldb/test/Shell/SymbolFile/DWARF/Inputs/inlined-file0-line0-col0.yaml
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/Inputs/inlined-file0-line0-col0.yaml
@@ -0,0 +1,699 @@
+--- !mach-o
+FileHeader:
+  magic:   0xFEEDFACF
+  cputype: 0x107
+  cpusubtype:  0x3
+  filetype:0x1
+  ncmds:   3
+  sizeofcmds:  736
+  flags:   0x2000
+  reserved:0x0
+LoadCommands:
+  - cmd: LC_SEGMENT_64
+cmdsize: 632
+segname: ''
+vmaddr:  0
+vmsize:  814
+fileoff: 768
+filesize:814
+maxprot: 7
+initprot:7
+nsects:  7
+flags:   0
+Sections:
+  - sectname:__text
+segname: __TEXT
+addr:0x0
+size:31
+offset:  0x300
+align:   4
+reloff:  0x630
+nreloc:  3
+flags:   0x8400
+reserved1:   0x0
+reserved2:   0x0
+reserved3:   0x0
+content: 554889E5833D067D0D833D067D0431C05DC3E8
+relocations:
+  - address: 0x1B
+symbolnum:   3
+pcrel:   true
+length:  2
+extern:  true
+type:2
+scattered:   false
+value:   0
+  - address: 0xF
+symbolnum:   1
+pcrel:   true
+length:  2
+extern:  true
+type:6
+scattered:   false
+value:   0
+  - address: 0x6
+symbolnum:   0
+pcrel:   true
+length:  2
+extern:  true
+type:6
+scattered:   false
+  

[Lldb-commits] [PATCH] D142552: [lldb] Make GetDIENamesAndRanges() allow 0-valued decl and call lines

[David Stenberg via Phabricator via lldb-commits]

dstenb added a comment.

First time looking at LLDB, so I had trouble finding a suitable way to create a 
reproducer for this. I would be happy to change to another type of lit test if 
that is more suitable.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142552

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

[Lldb-commits] [PATCH] D142552: [lldb] Make GetDIENamesAndRanges() allow 0-valued decl and call lines

[David Stenberg via Phabricator via lldb-commits]

dstenb added a comment.

In D142552#4083939 , @clayborg wrote:

> As long as there are no regressions in the test suite this looks good to me

Thanks, and sorry for the delay with landing this! I have ran check-lldb. I 
will land this shortly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142552

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

[Lldb-commits] [PATCH] D142552: [lldb] Make GetDIENamesAndRanges() allow 0-valued decl and call lines

[David Stenberg via Phabricator via lldb-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG98c3dc3fa748: [lldb] Make GetDIENamesAndRanges() allow 
0-valued decl and call lines (authored by dstenb).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142552

Files:
  lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDIE.h
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
  lldb/source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.h
  lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  lldb/test/Shell/SymbolFile/DWARF/Inputs/inlined-file0-line0-col0.yaml
  lldb/test/Shell/SymbolFile/DWARF/inlined-file0-line0-col0.test

Index: lldb/test/Shell/SymbolFile/DWARF/inlined-file0-line0-col0.test
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/inlined-file0-line0-col0.test
@@ -0,0 +1,34 @@
+# RUN: yaml2obj %S/Inputs/inlined-file0-line0-col0.yaml -o %t
+# RUN: %lldb %t -s %s -o exit | FileCheck %s
+
+#  1 void abort(void);
+#  2 int g1 = 4, g2 = 6;
+#  3
+#  4 inline __attribute__((always_inline)) void bar(int q) {
+#  5   if (q > 5)
+#  6 abort();
+#  7 }
+#  8
+#  9 inline __attribute__((always_inline)) void foo(int q) {
+# 10   bar(q);
+# 11 }
+# 12
+# 13 int main() {
+# 14   foo(g1);
+# 15   foo(g2);
+# 16   return 0;
+# 17 }
+
+# The input object file contains a single abort invocation for the two inlined
+# instances of foo() in main() at line 0. As the file, line and column numbers
+# are all 0, file and line number information would be missing for foo and main
+# in the lookup information.
+#
+# A line number 0 is not printed for main in this case, but the same holds
+# for a non-inlined location with line number 0.
+
+# CHECK: Summary: inlined-file0-line0-col0.test.tmp`main + 30 [inlined] bar + 4 at inlined-file0-line0-col0.c:6:5
+# CHECK-NEXT: inlined-file0-line0-col0.test.tmp`main + 26 [inlined] foo at inlined-file0-line0-col0.c:10:3
+# CHECK-NEXT: inlined-file0-line0-col0.test.tmp`main + 26 at inlined-file0-line0-col0.c
+
+image lookup -a 0x1e
Index: lldb/test/Shell/SymbolFile/DWARF/Inputs/inlined-file0-line0-col0.yaml
===
--- /dev/null
+++ lldb/test/Shell/SymbolFile/DWARF/Inputs/inlined-file0-line0-col0.yaml
@@ -0,0 +1,699 @@
+--- !mach-o
+FileHeader:
+  magic:   0xFEEDFACF
+  cputype: 0x107
+  cpusubtype:  0x3
+  filetype:0x1
+  ncmds:   3
+  sizeofcmds:  736
+  flags:   0x2000
+  reserved:0x0
+LoadCommands:
+  - cmd: LC_SEGMENT_64
+cmdsize: 632
+segname: ''
+vmaddr:  0
+vmsize:  814
+fileoff: 768
+filesize:814
+maxprot: 7
+initprot:7
+nsects:  7
+flags:   0
+Sections:
+  - sectname:__text
+segname: __TEXT
+addr:0x0
+size:31
+offset:  0x300
+align:   4
+reloff:  0x630
+nreloc:  3
+flags:   0x8400
+reserved1:   0x0
+reserved2:   0x0
+reserved3:   0x0
+content: 554889E5833D067D0D833D067D0431C05DC3E8
+relocations:
+  - address: 0x1B
+symbolnum:   3
+pcrel:   true
+length:  2
+extern:  true
+type:2
+scattered:   false
+value:   0
+  - address: 0xF
+symbolnum:   1
+pcrel:   true
+length:  2
+extern:  true
+type:6
+scattered:   false
+value:   0
+  - address: 0x6
+symbolnum:   0
+pcrel:   true
+length:  2
+extern:  true
+type:6
+scattered:   false
+value:   0
+  - sectname:__data
+segname: __DATA
+addr:0x20
+size:8
+offset:  0x320
+align:   2
+reloff:  0x0
+nreloc:  0
+flags:   0x0
+reserved1:   0x0
+reserved2:   0x0
+reserved3:   0x0
+content: '04000600'
+  - sectname:__debug_abbrev
+segname: __DWARF
+addr:0x28
+size:182
+offset:  0x328
+align:   0
+reloff:  0x0
+

[Lldb-commits] [PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

[David Stenberg via Phabricator via lldb-commits]

dstenb added inline comments.



Comment at: llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp:870
 if (MI->isCandidateForCallSiteEntry() &&
-DAG->getTarget().Options.EnableDebugEntryValues)
+DAG->getTarget().Options.SupportsDebugEntryValues)
   MF.addCallArgsForwardingRegs(MI, DAG->getSDCallSiteInfo(Node));

I'm sorry for commenting on this so late, but when now adapting this patch for 
our downstream target, for which we will not enable call site info by default 
yet, I noticed that the call site information wasn't added. I think that this 
should be `ShouldEmitDebugEntryValues()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73534



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

[Lldb-commits] [PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

[David Stenberg via Phabricator via lldb-commits]

dstenb added inline comments.



Comment at: llvm/lib/CodeGen/SelectionDAG/ScheduleDAGSDNodes.cpp:870
 if (MI->isCandidateForCallSiteEntry() &&
-DAG->getTarget().Options.EnableDebugEntryValues)
+DAG->getTarget().Options.SupportsDebugEntryValues)
   MF.addCallArgsForwardingRegs(MI, DAG->getSDCallSiteInfo(Node));

djtodoro wrote:
> dstenb wrote:
> > I'm sorry for commenting on this so late, but when now adapting this patch 
> > for our downstream target, for which we will not enable call site info by 
> > default yet, I noticed that the call site information wasn't added. I think 
> > that this should be `ShouldEmitDebugEntryValues()`.
> I thought to restrict the production of the call site info here and only to 
> allow a hand-made testing, but I think you are right, we should use the 
> `ShouldEmitDebugEntryValues()`. I'll update the patch, thanks!
Thanks! It is perhaps a bit of a corner case, and we're moving towards enabling 
call site info by default, but I guess this might be useful when experimenting 
with adding call site info for other targets in the future.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D73534



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

[Lldb-commits] [PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

[David Stenberg via Phabricator via lldb-commits]

dstenb added a comment.

In D73534#1881353 , @nickdesaulniers 
wrote:

> As a heads up, Linaro's ToolChain Working Group's Linux kernel CI lit up on 
> this change. I see it's been reverted (thank you). Please cc me on the 
> updated patch and I can help test it against the Linux kernel.
>
> Edit: Link: 
> https://ci.linaro.org/job/tcwg_kernel-bisect-llvm-master-arm-mainline-allyesconfig/25/artifact/artifacts/build-a82d3e8a6e67473c94a5ce6345372748e9b61718/03-build_linux/console.log


Its hard to tell from the backtrace, but looking at the code, I think this 
might be a bug that sneaked in when I did D70431 
. Sorry if that is the case!

`ARMBaseInstrInfo::isAddImmediate()` does a `getReg()` without any `isReg()` 
guard:

  Optional ARMBaseInstrInfo::isAddImmediate(const MachineInstr &MI, 



 
Register Reg) const {   



 
  [...]
// TODO: Handle cases where Reg is a super- or sub-register of the  



 
// destination register.
if (Reg != MI.getOperand(0).getReg())
  return None;


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

https://reviews.llvm.org/D73534



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

[Lldb-commits] [PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

[David Stenberg via Phabricator via lldb-commits]

dstenb added a comment.

In D73534#1882127 , @djtodoro wrote:

> @dstenb No problem, we have already addressed the issue. Thanks to @vsk and 
> @nickdesaulniers! I'll update the patch.


Great, thanks!


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

https://reviews.llvm.org/D73534



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

[Lldb-commits] [PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

[David Stenberg via Phabricator via lldb-commits]

dstenb added a comment.

In D73534#1918940 , @dstenb wrote:

> In D73534#1918890 , @manojgupta 
> wrote:
>
> > Hi,
> >
> > I see another crash with this change when building gdb.
> >
> > Reduced test case:
> >  struct type *a(type *, type *, long, long);
> >  enum b {};
> >  static int empty_array(type *, int c) { type *d = a(__null, d, c, c - 1); }
> >  long e;
> >  b f() { empty_array(0, e); }
> >
> > Repros with:
> >  clang -cc1 -triple x86_64-linux-gnu -emit-obj -disable-free 
> > -mrelocation-model pic -pic-level 2 -pic-is-pie -mthread-model posix 
> > -mframe-pointer=all  -mconstructor-aliases -munwind-tables  
> > -dwarf-column-info  -debug-info-kind=limited -dwarf-version=4 
> > -debugger-tuning=gdb  -O2  -x c++
> >
> > Chromium bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1060788
>
>
> Oh, that assertion is related to D75036  
> which I did. I can have a look at that.


I wrote a PR for that: https://bugs.llvm.org/show_bug.cgi?id=45181.

I'll see if I'm able to put together something for that today.


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

https://reviews.llvm.org/D73534



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

[Lldb-commits] [PATCH] D73534: [DebugInfo] Enable the debug entry values feature by default

[David Stenberg via Phabricator via lldb-commits]

dstenb added a comment.

In D73534#1918890 , @manojgupta wrote:

> Hi,
>
> I see another crash with this change when building gdb.
>
> Reduced test case:
>  struct type *a(type *, type *, long, long);
>  enum b {};
>  static int empty_array(type *, int c) { type *d = a(__null, d, c, c - 1); }
>  long e;
>  b f() { empty_array(0, e); }
>
> Repros with:
>  clang -cc1 -triple x86_64-linux-gnu -emit-obj -disable-free 
> -mrelocation-model pic -pic-level 2 -pic-is-pie -mthread-model posix 
> -mframe-pointer=all  -mconstructor-aliases -munwind-tables  
> -dwarf-column-info  -debug-info-kind=limited -dwarf-version=4 
> -debugger-tuning=gdb  -O2  -x c++
>
> Chromium bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1060788


Oh, that assertion is related to D75036  which 
I did. I can have a look at that.


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

https://reviews.llvm.org/D73534



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