[PATCH] D71244: [DWARF5][DWARFVerifier] Check that Skeleton compilation unit does not have children.

[Alexey Lapshin via Phabricator via llvm-commits]

avl marked 5 inline comments as done.
avl added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:3542
       Args.hasFlag(options::OPT_fsplit_dwarf_inlining,
-                   options::OPT_fno_split_dwarf_inlining, true);
+                   options::OPT_fno_split_dwarf_inlining, false);
 
----------------
probinson wrote:
> dblaikie wrote:
> > aprantl wrote:
> > > probinson wrote:
> > > > aprantl wrote:
> > > > > What does this have to do with the dwarfdump patch?
> > > > So that by default we emit DWARF that passes the verifier?
> > > Sorry for being terse earlier. The review says this is a DWARFVerifier patch so it shouldn't touch clang. The clang part should be split into a separate commit with a different commit message.
> > +1 to this needing to be separate patches (& if there's any automation that's testing that some clang compiled binary passes the verifier that this verifier change would break, then the change in clang's defaults should go in first & then the verifier change after that to ensure things aren't broken between them)
> Oh, good point.  And a more explicit driver test, now that I look at it that way.
Ok, I would split the patch into two pieces.


================
Comment at: clang/test/Driver/split-debug.c:82
 
-// RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -gmlt -S -### %s 2> %t
+// RUN: %clang -target x86_64-unknown-linux-gnu -gsplit-dwarf -gmlt -fsplit-dwarf-inlining -S -### %s 2> %t 
 // RUN: FileCheck -check-prefix=CHECK-GMLT-OVER-SPLIT < %t %s
----------------
probinson wrote:
> There should be a test verifying that -fno-split-dwarf-inlining is the default.  Up around line 67 there's a test that passes it to the driver and then checks it gets passed to cc1; remove the option from that driver command line.
Ok.


================
Comment at: llvm/test/DebugInfo/skeleton-unit-verify.ll:1
+; RUN: llc %s -mtriple=x86_64-linux -filetype=obj -split-dwarf-cross-cu-references --split-dwarf-file=test.dwo -O3 -o %t 
+; RUN: llvm-dwarfdump --verify %t | FileCheck %s
----------------
dblaikie wrote:
> What does this functionality/test have to do with split-dwarf-cross-cu-references? That seems orthogonal/unrelated to this change? (& that feature is only related to LTO - this code change seems unrelated to LTO & the problematic cases would appear with or without LTO)
initially I created the test for two compilation units, default state of split-dwarf-cross-cu-references avoids -fsplit-dwarf-inlining thus I added it. Now it is redundant. would delete it.


================
Comment at: llvm/test/DebugInfo/skeleton-unit-verify.ll:3
+; RUN: llvm-dwarfdump --verify %t | FileCheck %s
+; RUN: sed 's/splitDebugInlining: false/splitDebugInlining: true/' < %s > %t2
+; RUN: llc %t2 -mtriple=x86_64-linux -filetype=obj -split-dwarf-cross-cu-references --split-dwarf-file=test.dwo -O3 -o %t1 
----------------
probinson wrote:
> aprantl wrote:
> > probinson wrote:
> > > There is no llc option to control this?  (Sorry, I'm lazy, haven't looked...)
> > You might want to use an assembler test instead. For broken debug info, that seems reasonable.
> We want to be able to emit compliant DWARF (with splitDebugInlining = false) and DWARF that is useful for backtraces in the absence of the .dwo file, that we properly detect as non-compliant (with splitDebugInlining = true).
>There is no llc option to control this? (Sorry, I'm lazy, haven't looked...)

right. there is no such an option.


================
Comment at: llvm/test/DebugInfo/skeleton-unit-verify.ll:9-21
+;int foo ( int p1 ) {
+;    return p1 + 10;
+;}
+;
+;int bar ( int p1 ) {
+;    return foo(p1);
+;}
----------------
dblaikie wrote:
> The simplest example would be:
> 
>   void f1();
>   __attribute__((always_inline)) void f2() {
>     f1();
>   }
>   void f3() {
>     f2();
>   }
> 
> That produces inlining without extra parameter types, etc, etc. Doesn't require optimizations & I'm not sure that "-gno-column-info" would be related/relevant (nor DWARFv5 - though I'm happy for split-dwarf related tests to be written in DWARFv5 by default so we can maybe remove the v4 support one day) - presumably the command line should have -gsplit-dwarf on it?
Ok, I will update the test accordingly. 

as to the -gsplit-dwarf - it is not required for generating .ll files by clang. for the llc there was set --split-dwarf-file=test.dwo.


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

https://reviews.llvm.org/D71244