This is an archive of the discontinued LLVM Phabricator instance.

[DebugInfo][dexter] Add dexter tests for escaped locals
ClosedPublic

Authored by Orlando on Oct 16 2020, 4:52 AM.

Details

Summary

Note: Some of these use -fno-inline, while others use inline attributes.

Recently there has been renewed interest in improving debug-info for variables that (partially or otherwise) live on the stack in optimised code.

At the moment instcombine speculates that stack slots are probably going to be promoted to registers, and prepares the debug-info accordingly. It runs a function called LowerDbgDeclare which converts dbg.declares to a set of dbg.values after loads, and before stores and calls. Sometimes the stack location remains (e.g. for escaped locals). If any dbg.values become undef where the stack location is still valid we end up unnecessarily reducing variable location coverage due to our inability to track multiple locations simultaneously. There is a flag to disable this feature (-instcombine-lower-dbg-declare=0), which prevents this conversion at the cost of sometimes providing incorrect location info in the face of DSE, DCE, GVN, CSE etc.

This has been discussed fairly extensively on PR34136.

The idea of these tests is to provide examples of situations that we should consider when designing a new system, to aid discussions and eventually help evaluate the implementation.

Dexter isn't ideal for observing specific optimisation behaviour. Writing an exaustive test suite would be difficult, and the resultant suite would be fragile. That said, I think having some concrete executable examples is useful at least as a reference, and I'm hoping that other people might contribute further examples.

What do you think?

Diff Detail

Event Timeline

Orlando requested review of this revision.Oct 16 2020, 4:52 AM
Orlando created this revision.
rnk added inline comments.Oct 16 2020, 12:12 PM
debuginfo-tests/dexter-tests/memvars/bitcast.c
4

I think I'd prefer to standardize on attributes or -fno-inline everywhere. I had to scroll back pretty far to the RUN line to understand that alias would not be inlined. Having the attribute be in the code may be more readable.

debuginfo-tests/dexter-tests/memvars/inlining.c
13

As far as I am aware, clang always supports the __attribute__ syntax, and the tests use clang cflags, so you could drop the _MSC_VER check.

debuginfo-tests/dexter-tests/memvars/ptr-to.c
23

Is this expectation saying that the user should not be able to observe the dead store of 0xB into local at label s2?

debuginfo-tests/dexter-tests/memvars/struct-dse.c
28

So, is this the expectation we want to hold, or one that does hold true currently? I would expect this to not hold for this test case, because as the comments say, we do DSE without running LowerDbgDeclare.

Thanks for taking a look @rnk. Your comment on struct-dse.c reminds me that I left out some important info in the description. All of these except ctrl-flow.c "fail" (score < 1.0) at the moment. Should they be marked XFAIL?

debuginfo-tests/dexter-tests/memvars/bitcast.c
4

SGTM. I will stick to attributes to make it clear when reading the source alone.

debuginfo-tests/dexter-tests/memvars/ptr-to.c
23

Yeah that's right, I'll add a comment.

debuginfo-tests/dexter-tests/memvars/struct-dse.c
28

The former: an expectation we want to hold. Most of these tests "fail" (get a scope lower than 1.0) currently.

Orlando added inline comments.Oct 19 2020, 7:49 AM
debuginfo-tests/dexter-tests/memvars/ptr-to.c
23

To be clear: Right now, stepping through s1 to s2 in a debugger, print local gives 0xB (good) but print *plocal gives 0xFF (misleading).

Orlando updated this revision to Diff 300319.Oct 23 2020, 9:13 AM
Orlando added a reviewer: TWeaver.
Orlando added a subscriber: llvm-commits.

Addressed Reid's comments:
+ Converted all tests to use attributes to control inlining behaviour (as opposed to a mix of this and clang flags).
+ Improved comments in some tests.

In addition:
+ Added implicit-ptr.c and inlining-dse.c, the problem in the latter is reported here: PR47946.
+ XFAILed the currently "failing" tests: bitcast.c, const-branch.c, implicit-ptr.c, inlining-dse.c, loop.c, ptr-to.c, struct-dse.c.
+ inlining.c now passes with D89810 landed.

rnk accepted this revision.Oct 23 2020, 2:59 PM

Thanks, it'll be good to have these tests written down.

This revision is now accepted and ready to land.Oct 23 2020, 2:59 PM
This revision was automatically updated to reflect the committed changes.
Orlando edited the summary of this revision. (Show Details)Oct 26 2020, 10:34 AM
Orlando marked 4 inline comments as done.Oct 26 2020, 10:36 AM

Thanks. I tried running these on Windows but ran into issues with the dexter args so I landed these with UNSUPPORTED: system-windows.