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.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
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? |
include/clang/Basic/Attr.td | ||
---|---|---|
86–88 ↗ | (On Diff #55692) | Actually not sure how to apply an attribute to an ImplicitParam.... |
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. (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. |
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. 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. |
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
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.
test/CodeGenCXX/debug-info-nodebug.cpp | ||
---|---|---|
50 ↗ | (On Diff #60930) | const case removed. |