That patch adds checking into DWARFVerifier that the Skeleton
compilation unit does not have children.
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
I like it, but the others should have a chance to say something.
| llvm/test/DebugInfo/skeleton-unit-verify.ll | ||
|---|---|---|
| 3 | There is no llc option to control this? (Sorry, I'm lazy, haven't looked...) | |
Comment Actions
The LLVM part LGTM, thanks!
| clang/lib/Driver/ToolChains/Clang.cpp | ||
|---|---|---|
| 3542 | What does this have to do with the dwarfdump patch? | |
| llvm/test/DebugInfo/skeleton-unit-verify.ll | ||
|---|---|---|
| 3 | You might want to use an assembler test instead. For broken debug info, that seems reasonable. | |
| clang/lib/Driver/ToolChains/Clang.cpp | ||
|---|---|---|
| 3542 | So that by default we emit DWARF that passes the verifier? | |
| llvm/test/DebugInfo/skeleton-unit-verify.ll | ||
| 3 | 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). | |
| clang/lib/Driver/ToolChains/Clang.cpp | ||
|---|---|---|
| 3542 | 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. | |
| clang/lib/Driver/ToolChains/Clang.cpp | ||
|---|---|---|
| 3542 | +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) | |
| llvm/test/DebugInfo/skeleton-unit-verify.ll | ||
| 1 | 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) | |
| 9–21 | 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? | |
| clang/lib/Driver/ToolChains/Clang.cpp | ||
|---|---|---|
| 3542 | Oh, good point. And a more explicit driver test, now that I look at it that way. | |
| clang/test/Driver/split-debug.c | ||
| 82 | 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. | |
| clang/lib/Driver/ToolChains/Clang.cpp | ||
|---|---|---|
| 3542 | Ok, I would split the patch into two pieces. | |
| clang/test/Driver/split-debug.c | ||
| 82 | Ok. | |
| llvm/test/DebugInfo/skeleton-unit-verify.ll | ||
| 1 | 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. | |
| 3 |
right. there is no such an option. | |
| 9–21 | 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. | |
| llvm/test/DebugInfo/X86/skeleton-unit-verify.ll | ||
|---|---|---|
| 3 ↗ | (On Diff #233318) | If you really want to use sed here (I think an assembler or yaml2obj test would be more appropriate for malformed dwarf) you need to add REQUIRES: shell |
| llvm/test/DebugInfo/X86/skeleton-unit-verify.ll | ||
|---|---|---|
| 3 ↗ | (On Diff #233318) | I now agree with Adrian, a hand-coded .debug_info section in assembler would be better. There are other tests that verify splitDebugInlining=true does the right thing. You will want one split CU with a tag that has no children, and one with a tag that does; the verifier should detect only the one error. |
What does this have to do with the dwarfdump patch?