Page MenuHomePhabricator

Allow 'nodebug' on local variables
ClosedPublic

Authored by probinson on Apr 29 2016, 5:43 PM.

Details

Summary

Expand the purview of the 'nodebug' attribute from functions/methods and static-storage variables to include local variables. Parameters and members of aggregates are still excluded, and probably should stay that way.

Diff Detail

Repository
rL LLVM

Event Timeline

probinson updated this revision to Diff 55692.Apr 29 2016, 5:43 PM
probinson retitled this revision from to Allow 'nodebug' on local variables.
probinson updated this object.
probinson added a subscriber: cfe-commits.
aaron.ballman added inline comments.May 2 2016, 5:46 AM
include/clang/Basic/Attr.td
86–88 ↗(On Diff #55692)

Can you add tests for each of these cases to ensure that the diagnostic fires on all of them?

probinson added inline comments.May 2 2016, 9:24 AM
include/clang/Basic/Attr.td
86–88 ↗(On Diff #55692)

Actually not sure how to apply an attribute to an ImplicitParam....

dblaikie added inline comments.May 2 2016, 11:52 AM
test/CodeGenCXX/debug-info-nodebug.cpp
50 ↗(On Diff #55692)

Doesn't look like the const case is any different from the non-const case, is it?

test/CodeGenObjC/debug-info-nodebug.m
17 ↗(On Diff #55692)

Is this case outside of the block interesting in some way? It doesn't look like it.

probinson marked 2 inline comments as done.May 3 2016, 4:38 PM
probinson added inline comments.
include/clang/Basic/Attr.td
86–88 ↗(On Diff #55692)

Well, this is all very exciting. I tried

template<__attribute__((nodebug)) int i> int t() { return i; }
int g() { return t<2>(); }

but got no diagnostic. In fact. putting assert(false) in handleNoDebugAttr shows that it's never called. Unsurprisingly, debug info for the template parameter still appears.
Maybe nobody has ever been so foolish as to try to put an attribute on a template parameter before? (It is mildly disturbing that putting an attribute in an apparently impossible place yields no diagnostic and no semantic effect.)
I was obviously modeling this check on NormalVar, which (it turns out) is never used. And if you can't put attributes on template parameters or (it would seem likely) implicit parameters, then NonParmVar should check for nothing more than ParmVar.
And that's what I'll do.

(Marking as Done because the set of tests now matches the specified condition.)

test/CodeGenCXX/debug-info-nodebug.cpp
50 ↗(On Diff #55692)

Given a white-box analysis of the compiler internals, you are correct; this is in contrast to the static const/non-const handling, which *does* use different paths.
I am unwilling to trust that the const/non-const paths for locals will forever use the same path. You could also look at it as "the test is the spec."

test/CodeGenObjC/debug-info-nodebug.m
17 ↗(On Diff #55692)

The attribute on "weakSelf" is what triggers the second modified path in CGDebugInfo and suppresses the DILocalVariable for that name.
The attribute on "strongSelf" goes through the normal EmitDeclare path. So in that sense, it is not interesting.

I should not have been so hesitant to work out what was going on here, sorry about that. My cluelessness about Objective-C knows no bounds.
I'm changing the test to verify that the DILocalVariable does not appear, while the member info does still appear, and updating the comment to reflect this new knowledge.

probinson updated this revision to Diff 56079.May 3 2016, 4:39 PM
probinson marked an inline comment as done.

Correct the attribute condition, and refine the Objective-C test.

aaron.ballman accepted this revision.May 4 2016, 12:13 PM
aaron.ballman edited edge metadata.

LGTM!

This revision is now accepted and ready to land.May 4 2016, 12:13 PM

This would be a great conversation to have at the social, sadly I will
have to miss it this month.

dblaikie wrote:

Doesn't look like the const case is any different from the non-const
case, is it?

Given a white-box analysis of the compiler internals, you are correct;
this is in contrast to the static const/non-const handling, which *does*
use different paths.
I am unwilling to trust that the const/non-const paths for locals will
forever use the same path.  You could also look at it as "the test is
the spec."

But even then - what about any other property of a variable? What if it's
in a nested scope instead of a top level scope? What if it's declared in
a condition (if (int x = ...)). What if it's volatile? We could pick
arbitrary properties that /could/ have an effect on debug info but we
generally believe to be orthogonal to the feature at hand

What you are describing is what testing literature refers to as criteria
for equivalence classes. There is some level of judgment to that, yes.

& I'd say 'const' is in the same group of things.

I would have thought exactly that for the static-storage case, but it is
demonstrably not true. Therefore, const/not is a valid distinction for
the equivalence classes in the static case. Needing a separate const
test for the static case, it seems completely appropriate to have the
same for the auto case. In other words, in my judgment the storage-class
doesn't seem relevant to the equivalence class criteria for the test.

(a const local variable still needs storage, etc, - the address can be
escaped and accessed from elsewhere and determined to be unique

All of those objections apply equally to the static case, and yet the
static case must have separate tests.

  • and both variables in this example could be optimized away entirely by the compiler because they're unused, so the const is no worse off there in that theoretical concern)

Again not different for the (file-)static case. If a file-static
variable is not used in the CU there's no reason for it to be emitted.
As it happens I *did* need uses to get the static cases to work, and
(currently) don't need uses to get the local cases to work, so in the
interest of not including elements in the test case that are irrelevant
to the feature-under-test, I didn't add uses of the locals.

This is an argument for doing the test exactly as I did: first run it
to prove debug info IS emitted in the absence of the attribute, then
again to prove debug info is suppressed in the presence of the attribute.
That way if optimization or lack-of-use means the variable is not emitted,
the test can be adjusted to make sure that condition does not apply,
proving that the lack of debug info is properly because of the attribute
and not because of some other irrelevant circumstance.

--paulr


cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

probinson updated this revision to Diff 60930.Jun 15 2016, 4:35 PM
probinson edited edge metadata.

Removed the apparently redundant test for a const local variable.

Yes, I'm back to this after way longer than expected. I believe, for this patch specifically, the extra const local variable was the only identifiable problem. There were other issues regarding the overall test, which were partly discussed here and partly in D19567, but those were not specific to this patch.

@dblaikie, I propose that with the 'const' test removed, we allow this patch to proceed (which completes the implementation, something I would dearly like to have done before 3.9 branches) with the promise that I'll come back around to the broader topic of the test file in general as a follow-up.

probinson marked an inline comment as done.Jun 15 2016, 4:36 PM
probinson added inline comments.
test/CodeGenCXX/debug-info-nodebug.cpp
50 ↗(On Diff #60930)

const case removed.

dblaikie accepted this revision.Jun 15 2016, 4:43 PM
dblaikie edited edge metadata.

Sure, looks good - thanks. Will discuss the broader test issues later/separately.

This revision was automatically updated to reflect the committed changes.