Page MenuHomePhabricator

[DebugInfo] Refactored macro related generation, added a test case for macinfo.dwo emission.
ClosedPublic

Authored by SouraVX on Dec 4 2019, 5:39 AM.

Details

Summary

This patch refactors some part of D70705, based on review suggestion and also adds a test case for emission of macinfo.dwo section.

Diff Detail

Event Timeline

SouraVX created this revision.Dec 4 2019, 5:39 AM

Build result: pass - 60453 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

Sorry, I'm a bit confused - this sort of thing should be included in the original review.

But I think this whole direction (supporting macinfo.dwo) should not be done, since macinfo is not in DWARFv5 - so to reach DWARFv5 support, you/we/someone should first fix the existing bug (which is that debug_macro should be emitted in DWARFv5, but currently LLVM produces debug_macinfo instead). and then that correct support can be expanded to support Split DWARFv5.

Sorry, I'm a bit confused - this sort of thing should be included in the original review.

Ah, that was showing accepted. Even you reopened it[I noticed]. So that's why I created a new one. I think, I mentioned my intentions in previous revision also ??

But I think this whole direction (supporting macinfo.dwo) should not be done, since macinfo is not in DWARFv5 - so to reach DWARFv5 support, you/we/someone should first fix the existing bug (which is that debug_macro should be emitted in DWARFv5, but currently LLVM produces debug_macinfo instead). and then that correct support can be expanded to support Split DWARFv5.

I'm working on DWARFv5 debug_macro also, holding up till this also gets it.
But Main motivation behind this are -- {Some mentioned in previous revision}

  • One of the major factor is GDB and how it handles stuff, if you try using clang 9.0 or any older version. behavior of macro is like this -g -gsplit-dwarf -fdebug-macro{-g defaulting to DWARF version 4}. debug_macinfo got populated{even when split-dwarf is specified}. Now load that binary in GDB, try expanding macros, it fails. that renders all the heavy lifting compiler did to produce debug_macinfo section is finally useless. Please note here that this behavior is contrary to GCC, GCC produces debug_macinfo.dwo when -gsplit-dwarf is specified. GDB also seems to be happy, macro expansion works fine. Seems like for split-dwarf mode, GDB looks for macinfo.dwo in DWO file. Just a side note GCC-9.2, I think trunk also, only generate macinfo/macinfo.dwo only when -gstrict-dwarf is also specified along with the usual ones, otherwise GCC by defaults generates macro/macro.dwo.
  • For a C++ project, the macro information, can be huge. moving this to separate file{DWO} can be a huge win. I think saves a bit of linking time also ?
  • Nevertheless, considering the present situation{assuming you/someone else don't want macinfo.dwo possibly revert it}. For binaries compiled with -g -gsplit-dwarf compiler will still produce debug_macinfo, that GDB cannot use.

I think, these seems points reasonable enough. I also want to here your opinion/motivation as to why should not be doing this.

dblaikie accepted this revision.Dec 5 2019, 7:47 PM

Sorry, I'm a bit confused - this sort of thing should be included in the original review.

Ah, that was showing accepted. Even you reopened it[I noticed]. So that's why I created a new one. I think, I mentioned my intentions in previous revision also ??

Ah, I saw it had been accepted, but didn't realize it had already been /committed/. Makes sense - sorry for the confusion.

But I think this whole direction (supporting macinfo.dwo) should not be done, since macinfo is not in DWARFv5 - so to reach DWARFv5 support, you/we/someone should first fix the existing bug (which is that debug_macro should be emitted in DWARFv5, but currently LLVM produces debug_macinfo instead). and then that correct support can be expanded to support Split DWARFv5.

I'm working on DWARFv5 debug_macro also, holding up till this also gets it.
But Main motivation behind this are -- {Some mentioned in previous revision}

  • One of the major factor is GDB and how it handles stuff, if you try using clang 9.0 or any older version. behavior of macro is like this -g -gsplit-dwarf -fdebug-macro{-g defaulting to DWARF version 4}. debug_macinfo got populated{even when split-dwarf is specified}. Now load that binary in GDB, try expanding macros, it fails. that renders all the heavy lifting compiler did to produce debug_macinfo section is finally useless. Please note here that this behavior is contrary to GCC, GCC produces debug_macinfo.dwo when -gsplit-dwarf is specified. GDB also seems to be happy, macro expansion works fine. Seems like for split-dwarf mode, GDB looks for macinfo.dwo in DWO file. Just a side note GCC-9.2, I think trunk also, only generate macinfo/macinfo.dwo only when -gstrict-dwarf is also specified along with the usual ones, otherwise GCC by defaults generates macro/macro.dwo.
  • For a C++ project, the macro information, can be huge. moving this to separate file{DWO} can be a huge win. I think saves a bit of linking time also ?
  • Nevertheless, considering the present situation{assuming you/someone else don't want macinfo.dwo possibly revert it}. For binaries compiled with -g -gsplit-dwarf compiler will still produce debug_macinfo, that GDB cannot use. I think, these seems points reasonable enough. I also want to here your opinion/motivation as to why should not be doing this.

Ah, you're looking to fix the existing DWARFv4 support, then? It might be better if the test case were for DWARFv4, rather than DWARFv5, then? Then it's less a case of "adding buggy functionality/replacing one bug with another" (debug_macinfo.dwo in DWARFv5 being buggy/invalid) and more a case of "fixing a bug" (debug_macinfo.dwo in DWARFv4 non-standard fission extension/pre-standard implementation)

https://gcc.gnu.org/wiki/DebugFission (the proposal/design document for Split DWARF) does specify the behavior of macros under Split DWARF, so it seems fair to me to implement/fix that (though I'm mostly of the opinion that fixing pre-DWARFv5 Split DWARF support is a low priority & we should be planning to remove that support to reduce the support surface) - but since you want to work on it/the patches are written/committed, so that's OK.

So, yeah, in short: Approved, conditional on changing the test case to DWARFv4 rather than DWARFv5. Thanks!

This revision is now accepted and ready to land.Dec 5 2019, 7:47 PM

Sorry, I'm a bit confused - this sort of thing should be included in the original review.

Ah, that was showing accepted. Even you reopened it[I noticed]. So that's why I created a new one. I think, I mentioned my intentions in previous revision also ??

Ah, I saw it had been accepted, but didn't realize it had already been /committed/. Makes sense - sorry for the confusion.

But I think this whole direction (supporting macinfo.dwo) should not be done, since macinfo is not in DWARFv5 - so to reach DWARFv5 support, you/we/someone should first fix the existing bug (which is that debug_macro should be emitted in DWARFv5, but currently LLVM produces debug_macinfo instead). and then that correct support can be expanded to support Split DWARFv5.

I'm working on DWARFv5 debug_macro also, holding up till this also gets it.
But Main motivation behind this are -- {Some mentioned in previous revision}

  • One of the major factor is GDB and how it handles stuff, if you try using clang 9.0 or any older version. behavior of macro is like this -g -gsplit-dwarf -fdebug-macro{-g defaulting to DWARF version 4}. debug_macinfo got populated{even when split-dwarf is specified}. Now load that binary in GDB, try expanding macros, it fails. that renders all the heavy lifting compiler did to produce debug_macinfo section is finally useless. Please note here that this behavior is contrary to GCC, GCC produces debug_macinfo.dwo when -gsplit-dwarf is specified. GDB also seems to be happy, macro expansion works fine. Seems like for split-dwarf mode, GDB looks for macinfo.dwo in DWO file. Just a side note GCC-9.2, I think trunk also, only generate macinfo/macinfo.dwo only when -gstrict-dwarf is also specified along with the usual ones, otherwise GCC by defaults generates macro/macro.dwo.
  • For a C++ project, the macro information, can be huge. moving this to separate file{DWO} can be a huge win. I think saves a bit of linking time also ?
  • Nevertheless, considering the present situation{assuming you/someone else don't want macinfo.dwo possibly revert it}. For binaries compiled with -g -gsplit-dwarf compiler will still produce debug_macinfo, that GDB cannot use. I think, these seems points reasonable enough. I also want to here your opinion/motivation as to why should not be doing this.

Ah, you're looking to fix the existing DWARFv4 support, then? It might be better if the test case were for DWARFv4, rather than DWARFv5, then? Then it's less a case of "adding buggy functionality/replacing one bug with another" (debug_macinfo.dwo in DWARFv5 being buggy/invalid) and more a case of "fixing a bug" (debug_macinfo.dwo in DWARFv4 non-standard fission extension/pre-standard implementation)

Sure, will be changing dwarf version in test case before committing, the approach I'm following for preparing patches is that. If it's not DWARFv5 related, I mark that one as [DebugInfo]. and [DWARF5] [DebugInfo] for the one related to DWARFv5.

https://gcc.gnu.org/wiki/DebugFission (the proposal/design document for Split DWARF) does specify the behavior of macros under Split DWARF, so it seems fair to me to implement/fix that (though I'm mostly of the opinion that fixing pre-DWARFv5 Split DWARF support is a low priority & we should be planning to remove that support to reduce the support surface) - but since you want to work on it/the patches are written/committed, so that's OK.

While analyzing/working on macro stuff, I stumbled upon this hole. So thought I should fix this first.

So, yeah, in short: Approved, conditional on changing the test case to DWARFv4 rather than DWARFv5. Thanks!

Thank you for this!

SouraVX marked an inline comment as done.Dec 5 2019, 11:32 PM
SouraVX added inline comments.
llvm/test/DebugInfo/X86/debug-macro-split-dwarf.ll
67 ↗(On Diff #232095)

@dblaikie are you suggesting to change DWARF version here, or renaming the test case it self to something like debug-macinfo-split-dwarf.ll.
Please note here, that their is a bit of inconsistency, I noticed a non-split DWARFv4 test case named as debug-macro.ll .
If that's what you're suggesting; then maybe we should also rename debug-macro.ll to debug-macinfo.ll for naming consistency and future addition of test cases.

dblaikie added inline comments.Dec 10 2019, 10:46 AM
llvm/test/DebugInfo/X86/debug-macro-split-dwarf.ll
67 ↗(On Diff #232095)

Yep, this is where I was suggesting changing the version.

Yeah, at the time the debug-macro.ll test was written there was no debug_macro section, so there was no ambiguity (debug-macro obviously tested the debug_macinfo section), but now that there is, it may be somewhat confusing & could be renamed (don't do that in this patch though).

Renaming this test before it's committed seems like a good idea to go with the version change, yes.

SouraVX marked an inline comment as done.Dec 10 2019, 12:00 PM
SouraVX added inline comments.
llvm/test/DebugInfo/X86/debug-macro-split-dwarf.ll
67 ↗(On Diff #232095)

Thank you for clarifying this.
Sure will do this, before committing,

This revision was automatically updated to reflect the committed changes.

This breaks tests on Windows: http://45.33.8.238/win/3783/step_11.txt

Please take a look, and if it takes a while to investigate, please revert for now.

This breaks tests on Windows: http://45.33.8.238/win/3783/step_11.txt

Please take a look, and if it takes a while to investigate, please revert for now.

Sorry, if that caused trouble. I'll be investigating this. BTW, this seems strange to me Harbour Build B41847: Diff 232095, here everything worked fine without a problem. Does this suggests Harbour master doesn't build and test for Windows ?

Sorry, if that caused trouble. I'll be investigating this. BTW, this seems strange to me Harbour Build B41847: Diff 232095, here everything worked fine without a problem. Does this suggests Harbour master doesn't build and test for Windows ?

As far as I know, that runs Linux only for now. I believe there are plans to add Windows soon-ish.

We saw the same failure on a different bot (https://ci.chromium.org/p/chromium/builders/try/win_upload_clang/777), so it should repro reliably on Windows.

Sorry for the trouble!