Page MenuHomePhabricator

[Debugify] Expose original debug info preservation check as CC1 option
ClosedPublic

Authored by djtodoro on Jun 25 2020, 6:52 AM.

Details

Summary

In order to test the preservation of the original Debug Info metadata in your projects, a front end option could be very useful, since users usually report that a concrete entity (e.g. variable x, or function fn2()) is missing debug info. The [0] is an example of running the utility on GDB Project.

Depends on: D82546 and D82545.

[0] https://djolertrk.github.io/di-checker-html-report-example/

Diff Detail

Event Timeline

djtodoro created this revision.Jun 25 2020, 6:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 25 2020, 6:52 AM
aprantl added inline comments.Jun 26 2020, 11:30 AM
clang/test/DebugInfo/debugify-each-original.c
57 ↗(On Diff #273338)

We probably shouldn't check the effects of LLVM passes in the Clang test suite. When someone breaks one of those LLVM passes and that shouldn't cause an error in the Clang *frontend* test suite. It's sufficient to check that the pass is passed on to LLVM here. The operation of it should be checked either in LLVM or in an end-to-end test, e.g., inside debug-info-tests.

aprantl added inline comments.Jun 26 2020, 11:33 AM
clang/test/DebugInfo/debugify-each-original.c
57 ↗(On Diff #273338)

I probably should emphasize that an end-to-end test will probably just cause us trouble: Someone will make an unrelated change to a pass, that pushes a subsequent pass into a behavior that doesn't pass the debugify verifier, and now the author of the unrelated patch is on the hook to fix that other pass. I think that will make everyone unhappy. Maybe it should be treated more like the debug info statistics?

djtodoro updated this revision to Diff 274054.Jun 29 2020, 4:24 AM

-Update the test

djtodoro marked an inline comment as done.Jun 29 2020, 4:31 AM
djtodoro added inline comments.
clang/test/DebugInfo/debugify-each-original.c
57 ↗(On Diff #273338)

@aprantl Thanks for your comments!

I agree with this... I refactored the test a bit, please take look.

Maybe it should be treated more like the debug info statistics?

The final goal of this is to be used either from the python script that should process the JSON objects (shared within previous patch), or the JSON objects could be used from http://green.lab.llvm.org/green/view/LLDB/job/clang-3.4-debuginfo-statistics/ as well with some additional (groovy) scripts.

aprantl added inline comments.Jun 29 2020, 4:33 PM
clang/test/DebugInfo/debugify-each-original.c
16 ↗(On Diff #274054)

I think this is still implicitly hardcoding the pass pipeline just through the order of CHECK lines. If there were a way to dump the flags Clang is passing to LLVM and check that, or get the pass manager to dump its configuration, that would be better. I'm not sure if there is such an affordance.

djtodoro marked an inline comment as done.Jun 30 2020, 5:18 AM
djtodoro added inline comments.
clang/test/DebugInfo/debugify-each-original.c
16 ↗(On Diff #274054)

I see... It is hard to test it that way, but as far as I can see, it is enough to test only the Driver in situations like this, so I am adding a new test case and getting rid of this one.

djtodoro updated this revision to Diff 274433.Jun 30 2020, 5:20 AM
  • Add the Driver test
  • Remove the old high level test in order to avoid troubles when someone updates an LLVM pass with the impact on Debugify output (e.g. a new debug info bugs)
  • clang-formatted
aprantl accepted this revision.Jun 30 2020, 10:34 AM
aprantl added inline comments.
clang/test/Driver/debugify-each-original.c
9 ↗(On Diff #274433)

I think this is redundant?

This revision is now accepted and ready to land.Jun 30 2020, 10:34 AM
djtodoro marked an inline comment as done.Jul 1 2020, 12:16 AM
djtodoro added inline comments.
clang/test/Driver/debugify-each-original.c
9 ↗(On Diff #274433)

It is, thanks!

djtodoro updated this revision to Diff 274690.Jul 1 2020, 12:18 AM
  • Remove redundant line from the test
vsk requested changes to this revision.Jul 5 2020, 5:24 PM
vsk added inline comments.
clang/lib/CodeGen/BackendUtil.cpp
933

Please factor out OptCustomPassManager from opt and generalize it so it can be used by both opt and clang. That should help ensure that extensions and bug fixes are only made to one custom 'debugify' pass manager.

971

I don't think the discussion from 'RFC: Introduce LLVM DI Checker utility' is complete, and I'd ask that you split off changes for 'original mode' from this patch until there's some consensus about what that mode should look like.

There are open questions about to what extent a new mode is needed (e.g., it may be that the interesting questions compiler developers need to answer about debug info loss are simpler to determine some other way (which is not to say that that's true -- just that we haven't explored the space much yet)). Or what its output should look like.

This revision now requires changes to proceed.Jul 5 2020, 5:24 PM
djtodoro marked 2 inline comments as done.Jul 6 2020, 6:37 AM
djtodoro added inline comments.
clang/lib/CodeGen/BackendUtil.cpp
933

I'll try that with the latest code. I remember I've tried it once, but I ended up moving it into the IR library (since we need to link it within legacy pass manager).

971

OK, I'll split off this and notify you/resend a message on the RFC.

djtodoro marked an inline comment as done.
djtodoro added inline comments.
clang/lib/CodeGen/BackendUtil.cpp
933

Hi @vsk, I've posted the patch as D83391. Thanks!

djtodoro updated this revision to Diff 276398.Jul 8 2020, 6:01 AM
djtodoro updated this revision to Diff 292469.Sep 17 2020, 5:28 AM
  • Rebasing
djtodoro retitled this revision from [Debugify] Expose debugify (original mode) as CC1 option to [VerifyDIPreserve] Expose original debuginfo preservation check as CC1 option.Sep 17 2020, 5:30 AM
djtodoro updated this revision to Diff 294658.Sep 28 2020, 4:51 AM

-Rebasing

djtodoro updated this revision to Diff 326005.Feb 24 2021, 12:37 AM
djtodoro retitled this revision from [VerifyDIPreserve] Expose original debuginfo preservation check as CC1 option to [Debugify] Expose original debug info preservation check as CC1 option.
  • Rebase on top of trunk
djtodoro updated this revision to Diff 329972.Mar 11 2021, 8:03 AM
  • rebase on top of trunk
  • refactor the code

ping :)

Apart from the one comment, LGTM.

clang/lib/Frontend/CompilerInvocation.cpp
1651–1657

Any particular behaviour if the user specifies a file for -fverify-debuginfo-preserve-export but doesn't set -fverify-debuginfo-preserve? It seems like it would be worth emitting a warning in this case, though I'm not sure if that's an established precedent.

jansvoboda11 requested changes to this revision.Tue, Mar 23, 2:37 AM
jansvoboda11 added inline comments.
clang/include/clang/Driver/Options.td
4884

Please, update the new options to use the marshalling infrastructure. You can then remove the code from CompilerInvocation.

https://clang.llvm.org/docs/InternalsManual.html#adding-new-command-line-option

This revision now requires changes to proceed.Tue, Mar 23, 2:37 AM
djtodoro added inline comments.Tue, Mar 23, 8:04 AM
clang/include/clang/Driver/Options.td
4884

Sure. Thanks!

clang/lib/Frontend/CompilerInvocation.cpp
1651–1657

it makes sense

djtodoro updated this revision to Diff 332679.Tue, Mar 23, 8:05 AM
  • addressing comments
jansvoboda11 resigned from this revision.Wed, Mar 24, 2:27 AM

Thanks for the update. Changes touching the command line LGTM, but I'll let others confirm the changes to CodeGen are fine too.

Thanks.

I feel like this is ready to go. Any additional comment?

jmorse added a subscriber: jmorse.Thu, Mar 25, 3:42 AM

FTR, the patch LGTM. I haven't really been following the development of debugify in detail so I have no feeling for whether this is good from a high level; if you're confident the mailing list discussion finished with this being the right direction, I'd say this is ready to land.

This revision was not accepted when it landed; it landed in state Needs Review.Thu, Mar 25, 5:31 AM
This revision was automatically updated to reflect the committed changes.