Page MenuHomePhabricator

[DWARF] Omitting the explicit import of an anonymous namespace is a debugger-tuning decision, not a target decision.
AbandonedPublic

Authored by probinson on Jan 4 2016, 6:02 PM.

Details

Summary

GDB and LLDB don't mind if we omit the explicit import of an anonymous namespace.

Diff Detail

Event Timeline

probinson updated this revision to Diff 43953.Jan 4 2016, 6:02 PM
probinson retitled this revision from to [DWARF] Omitting the explicit import of an anonymous namespace is a debugger-tuning decision, not a target decision..
probinson updated this object.
probinson added reviewers: echristo, dblaikie, aprantl.
probinson added a subscriber: cfe-commits.

Ping. I did want to get this finished off (moving all triple-based decisions to tuning-based decisions) before 3.8 branches.

echristo added inline comments.Jan 7 2016, 3:22 PM
lib/Frontend/CompilerInvocation.cpp
378–385

This seems like overkill for a single case.

probinson updated this revision to Diff 44278.Jan 7 2016, 3:42 PM
probinson marked an inline comment as done.
echristo added inline comments.Jan 7 2016, 3:45 PM
lib/Frontend/CompilerInvocation.cpp
446–448

Why not just a positive for debugger tuning SCE?

probinson added inline comments.Jan 7 2016, 4:04 PM
lib/Frontend/CompilerInvocation.cpp
378–385

I suppose... having the big hairy test inline isn't *that* unreadable.

446–448

Because the default (i.e., no tuning specified) behavior should be to conform to the DWARF spec, which basically says you need the explicit import. There's a new extra RUN line in the test, with no tuning specified, to verify this.
GDB and LLDB are the oddballs here, they implement a special case for namespaces whose name meets certain criteria, and do something beyond what DWARF says to do. So, the condition is written to express that.

FTR, the previous wiring-up-thru-Clang patches put the there-must-be-a-tuning decision in the driver; CC1 does not assume or require a non-default tuning.

echristo added inline comments.Jan 7 2016, 4:47 PM
lib/Frontend/CompilerInvocation.cpp
446–448

I don't necessarily agree with that interpretation on the explicit import - I did skim the thread, perhaps you could highlight what makes you think this?

probinson added inline comments.Jan 7 2016, 5:24 PM
lib/Frontend/CompilerInvocation.cpp
446–448

Basically, a namespace is a "context" for declarations, and the DWARF mechanism for making declarations from one context available in another context is with DW_TAG_imported_declaration and DW_TAG_imported_module.
The C++ spec describes the behavior "as if" there was an explicit using directive, and DW_TAG_imported_module is the DWARF mechanism for describing a using directive.

The meaning of DWARF is determined by the DWARF spec, not the C++ spec, and the DWARF spec does not say there's anything special about a namespace that has no name. There is a perfectly reasonable DWARF mechanism for getting the desired effect, so there's no reason for DWARF to make a special rule for an unnamed namespace. Therefore, an anonymous namespace should be explicitly imported into the containing namespace. The explicit import would be marked artificial of course.

probinson added inline comments.Jan 12 2016, 8:56 AM
lib/Frontend/CompilerInvocation.cpp
446–448

Ping. Have I missed something in the DWARF spec that makes you think my interpretation is incorrect? Wouldn't be the first time...

Ping. FWIW in today's DWARF meeting, we agreed that the Appendix D example of an anonymous namespace should have an explicit import from the containing scope (see DWARF v4, Figure 57 on p.233; it was essentially unchanged in the v5 draft).

echristo added inline comments.Feb 4 2016, 1:59 PM
lib/Frontend/CompilerInvocation.cpp
446–448

I don't have anything to add to the reasoning the David has given you. We both agree and let's just make this a positive tuning for SCE.

probinson marked an inline comment as done.Feb 4 2016, 2:34 PM
probinson added inline comments.
lib/Frontend/CompilerInvocation.cpp
446–448

Fine. It'll be on just for SCE.
// Pedantic DWARF requires explicit import but only SCE insists.

echristo added inline comments.Feb 4 2016, 2:36 PM
lib/Frontend/CompilerInvocation.cpp
446–448

Please don't add that comment to this, I don't believe that it is valid or useful.

probinson marked an inline comment as done.Feb 4 2016, 2:43 PM
probinson added inline comments.
lib/Frontend/CompilerInvocation.cpp
446–448

The DWARF committee disagrees with your validity opinion, but I will take the comment out.

echristo added inline comments.Feb 4 2016, 2:43 PM
lib/Frontend/CompilerInvocation.cpp
446–448

Bring it up on the list then.

probinson added inline comments.Feb 4 2016, 2:54 PM
lib/Frontend/CompilerInvocation.cpp
446–448

I brought it up during the document review; see my ping comment from Jan 28.

echristo added inline comments.Feb 4 2016, 2:55 PM
lib/Frontend/CompilerInvocation.cpp
446–448

I saw. If you want that to be a required element of the spec then we need to change multiple wordings in the DWARF spec. I think the right place to bring that up is with the committee. The example is perfectly valid DWARF with the import, it's just unnecessary.

probinson abandoned this revision.Dec 10 2018, 2:11 PM

Abandoning dead patch. This wound up being done a different way.