Page MenuHomePhabricator

[Debugify] Expose debugify (original mode) as CC1 option
Needs ReviewPublic

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
10

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
10

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
855

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.

893

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
855

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).

893

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
855

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

djtodoro updated this revision to Diff 276398.Jul 8 2020, 6:01 AM