Page MenuHomePhabricator

Anonymous namespaces are missing import DW_TAG_imported_module.
AbandonedPublic

Authored by kromanova on Feb 25 2015, 7:06 PM.

Details

Summary

DW_TAG_imported_module is missing from the compile unit's root scope. Without this tag, anonymous namespace is not imported into the module scope.

Consider the following testcase. Lack of DW_TAG_imported_module prevents our debugger from displaying the value of ‘a’.
It looks like GDB imports the anonymous namespace automatically, so it does not have this problem.
If necessary, I could add an option to control generation of DW_TAG_imported_module for anonymous namespaces.

#include <stdio.h>

namespace
{

int a = 5;

}

int main()
{

printf("%d\n", a);

return 0;

}

Diff Detail

Event Timeline

kromanova updated this revision to Diff 20725.Feb 25 2015, 7:06 PM
kromanova retitled this revision from to Anonymous namespaces are missing import DW_TAG_imported_module..
kromanova updated this object.
kromanova edited the test plan for this revision. (Show Details)
kromanova added reviewers: probinson, dblaikie, echristo.
kromanova added a subscriber: Unknown Object (MLST).

I have already created a review for this bug a few hours ago, but it didn't reach the full audience. I didn't add llvm--dev at first when I created the review. Apparently, adding it later doesn't have any effect and the review wasn't sent out to the mailing list. I abandoned the old review and created this one.

Here are a couple of comments that were added to the old review by the people who were specified as the reviewers:

from David Blaikie:

I'm not sure if this is really a bug in the debug info - seems like it might be reasonably treated as a bug in the debugger?

from Paul Robinson:

Our debugger guys get pretty pedantic about interpreting the DWARF spec. I know GDB will auto-import info from an anonymous namespace, but:

the size cost here is really minimal (what, 5 or 6 bytes per anonymous namespace scope);
they claimed that deciding whether to skip the namespace based on the name would be a problem for them;
the compiler implementation is obviously trivial;

so I didn't really feel motivated to argue the point.

Hi,
I'm having technical problems with Phabricator. Per Duncun request, I attached a patch for the review http://reviews.llvm.org/D7895
Thanks!
Katya.

kromanova updated this revision to Diff 20824.Feb 26 2015, 9:08 PM

Few minor changes:
(1) I added DW_AT_artificial attribute to DW_TAG_imported_module.
(2) I checked in anonymous-namespace.ll that DW_AT_artificial attribute is generated
(3) I renamed the local variable from Anonymous to Is Anonymous (per Duncan's suggestion).
(4) I fixed copy and paste error in the anonymous-namespace.ll testcase.

Please review.

As per Fred's request, I made sure that my change didn't break neither GDB (versions 7.5, 7.7 and 7.9), nor LLDB (3.6.0). Since GCC also generates DW_TAG_import_module, this was expected.

I wanted to point out that that GCC compiler and LLVM compiler produce slightly different DWARF. Is our generation OK?

GCC
<1><73>: Abbrev Number: 4 (DW_TAG_namespace)
<74> DW_AT_sibling : <0x82>
...
<1><82>: Abbrev Number: 6 (DW_TAG_imported_module)
<83> DW_AT_decl_file : 1
<84> DW_AT_decl_line : 4
<85> DW_AT_import : <0x73> [Abbrev Number: 4 (DW_TAG_namespace)]

LLVM:
<1><2a>: Abbrev Number: 2 (DW_TAG_namespace)
<2b> DW_AT_decl_file : 1
<2c> DW_AT_decl_line : 4
...
<1><47>: Abbrev Number: 4 (DW_TAG_imported_module)
<48> DW_AT_import : <0x2a> [Abbrev Number: 2 (DW_TAG_namespace)]
<4c> DW_AT_artificial : 1

dblaikie edited edge metadata.Feb 26 2015, 11:29 PM

The differences you're pointing out are these?

  • GCC doesn't put line/file on a namespace, Clang does
  • GCC adds DW_AT_sibling, Clang doesn't
  • GCC adds line/file on imported_module, Clang doesn't
  • GCC omits artificial from the imported_module, Clang doesn't

On those:

  • Clang should omit them for brevity
  • Clang omits these for brevity, some consumers might find it easier to

parse if they were present

  • Clang could add these, I'm not sure if any debugger cares - notionally

they could do more accurate expression evaluation with that information
(not using the using directive when the user is evaluating an expression in
a context lexically prior to the using directive)

  • Unlikely to matter.

Hi David,

Yes, these are exactly 4 differences that I wanted to point out.
The questionable differences are #1 and #3.

  • GCC doesn't put line/file on a namespace, Clang does. Clang should omit them for brevity

Should I remove generation of line/file names for anonymous namespaces before I commit my change?

--GCC adds line/file on imported_module, Clang doesn't. Clang could add these, I'm not sure if any debugger cares - notionally they could do more accurate expression evaluation with that information (not using the using directive when the user is evaluating an expression in a context lexically prior to the using directive).
Our debugger doesn't care. I'm not sure if other debuggers care. So, I won't do any changes with respect of this.

Is my latest patch look OK or should I change anything?

Hi David,

Yes, these are exactly 4 differences that I wanted to point out.
The questionable differences are #1 and #3.

  • GCC doesn't put line/file on a namespace, Clang does. Clang should omit them for brevity Should I remove generation of line/file names for anonymous namespaces before I commit my change?

No - it seems like a simple orthogonal change that could go in before/after/whenever someone decides they care about it.

--GCC adds line/file on imported_module, Clang doesn't. Clang could add these, I'm not sure if any debugger cares - notionally they could do more accurate expression evaluation with that information (not using the using directive when the user is evaluating an expression in a context lexically prior to the using directive).
Our debugger doesn't care. I'm not sure if other debuggers care. So, I won't do any changes with respect of this.

*nod* if someone files a bug one day because a debugger cares, I'm happy to worry about it then.

Is my latest patch look OK or should I change anything?

I still don't know if it's the right thing to do - I'm still sort of inclined to treating this as a debugger bug & avoiding the extra code/complexity in LLVM (though it's not a lot, of course).

test/DebugInfo/anonymous-namespace.ll
12 ↗(On Diff #20824)

You could simplify this to "return a"

It might be even simpler/smaller IR to drop the 'foo' function and just keep 'a' alive with a global variable:

int *b = &a;
probinson edited edge metadata.Feb 27 2015, 5:28 PM

It's an arguable gray area in the compiler/debugger contract. How much
does the compiler _require_ the debugger to know? (Which is different
from how much the debugger knows for the purpose of improving the user's
debugging experience.)

This change reduces that requirement, with very low size cost (which we
are pretty sensitive to). A debugger that auto-imports anonymous
namespaces will see the import and recognize that it has already imported
that scope, so the cost there is also very low.

Not seeing a whole lot of down-side here.

kromanova updated this revision to Diff 20928.Feb 28 2015, 12:27 AM
kromanova edited edge metadata.

Reduced the test, based on David's suggestion.

The language says things in an anonymous namespace are automatically imported into the containing scope. The question is whether the compiler expects the debugger to know that a-priori.
The compiler can let the debugger off the hook by emitting an explicit import. Source fidelity requires that the import be marked artificial, because it's implicit in the source, rather than explicit; one of those "as if" things.
Or, the compiler can require the debugger to know more about how the language works, and not emit the explicit import.
Either way is legal DWARF, and has no source-fidelity issues, but the former doesn't leave one particular language detail up to the debugger to get right.
--paulr

From: llvm-commits-bounces@cs.uiuc.edu [mailto:llvm-commits-bounces@cs.uiuc.edu] On Behalf Of David Blaikie
Sent: Tuesday, March 03, 2015 9:44 AM
To: reviews+D7895+public+7827be49c0b04087@reviews.llvm.org
Cc: llvm-commits@cs.uiuc.edu; Romanova, Katya
Subject: Re: [PATCH] Anonymous namespaces are missing import DW_TAG_imported_module.

Hi Adrian,

Removing the line/file names would be great, because it would solve the LTO bug described in

http://lists.cs.uiuc.edu/pipermail/llvmdev/2014-June/073801.html

I could modify my patch to remove line/file names for anonymous namespaces.
BTW, was this bug fixed? I can't reproduce this assertion with the latest compiler...

but we need to be careful to not accidentally unique anonymous namespaces as outlined later in that thread.

How could I make sure that accidental uniquing is not happening? I simply wanted to skip generation of line/file number info for the anonymous namespace. Something like that (see below). Am I missing something?

  • addSourceLine(NDie, NS);

+ if (IsAnonymous) {
+ For anonymous namespaces, add a DW_TAG_imported_module tag
+
containing a DW_AT_import attribute with the reference to this
+ // namespace entry.
+ DIE &IDie = createAndAddDIE(dwarf::DW_TAG_imported_module, *ContextDIE);
+ addDIEEntry(IDie, dwarf::DW_AT_import, NDie);
+ addFlag(IDie, dwarf::DW_AT_artificial);
+ }
+ else
+ addSourceLine(NDie, NS);

return &NDie;

Thanks!
Katya.

Hi David,

I wanted to follow up on this bug. I still haven’t heard a definite “yes” or “no” from the other developers.

Here are my thought on why adding DW_TAG_imported_module is beneficial:

(1) We will be more consistent with debug info generated by other compilers (e.g. GCC and Intel's ICC).
(2) It resolves ambiguity created by lack of specification. LLDB and GDB are not the only debuggers in the world. Since it's a grey area in the DWARF spec, other debuggers might not automatically import the anonymous namespaces.

About your concerns:
(1) Import is implicit. It's marked as "artificial", so it shouldn't be confusing for the debuggers.
(2) The impact of the size increase is minimal, 6 bytes per anonymous namespace.
(3) The added complexity is very small. The patch is tiny and clear.

I think the benefits clearly outweigh the drawbacks.

We are talking about adding just one tag and a couple of attributes in some rare cases (like anonymous array). From what I remember, there are several existing attributes that are not exactly universally used, e.g. the DW_AT_APPLE_* attributes that are generated for all platforms, I'm not sure if anybody outside Apple has any use for it. And I haven't seen anybody raising any concerns related to size increases stemming from these attributes. . And we're just asking to add 6 bytes per anonymous namespace.

Sony’s debugger is just asking for a tiny bit of information from the compiler. It's not much really, but without this help. Fixing this problem in the debugger would be much harder. It might help other debuggers as well.

Katya.

From: David Blaikie [mailto:dblaikie@gmail.com]
Sent: Tuesday, March 03, 2015 9:44 AM
To: reviews+D7895+public+7827be49c0b04087@reviews.llvm.org
Cc: Romanova, Katya; Eric Christopher; Frédéric Riss; Duncan P. N. Exon Smith; llvm-commits@cs.uiuc.edu
Subject: Re: [PATCH] Anonymous namespaces are missing import DW_TAG_imported_module.

kromanova updated this revision to Diff 22164.Mar 18 2015, 1:13 AM

Added PS4 flag (as David requested).
Modified the tests accordingly.

kromanova updated this revision to Diff 22203.Mar 18 2015, 12:23 PM

Added the negative test for non-PS4 triple.
Moved the test to X86 directory.

dblaikie accepted this revision.Mar 18 2015, 12:46 PM
dblaikie edited edge metadata.

Some follow-up comments you can address before/after committing.

Only other thing I could think of is that this could be moved to the frontend (clang) - since it's a language-specific workaround, so it should perhaps be where the language semantics are known rather than polluting LLVM with more language-specific knowledge it could avoid having.

But if you like that idea, feel free to do/not do it separately from this patch (ie: if you want this in in the interim while working on a frontend patch for this, then remove this backend change afterwards).

lib/CodeGen/AsmPrinter/DwarfUnit.cpp
1234 ↗(On Diff #22203)

Probably switch the condition around (test Name.empty first) to do the cheaper thing first (so the more expensive thing can be skipped in some cases)

test/DebugInfo/X86/anonymous-namespace.ll
37 ↗(On Diff #22203)

Probably move all the check lines up above the IR so the test file is easier to read

41 ↗(On Diff #22203)

I'd just leave the first of these 3 lines here, the other two don't seem important if we didn't create the tag in the first place.

This revision is now accepted and ready to land.Mar 18 2015, 12:46 PM

Hi David,
If you insist on moving the patch to the FE, I would rather do it now than later.

Katya.

kromanova updated this revision to Diff 22448.Mar 23 2015, 1:47 AM
kromanova edited the test plan for this revision. (Show Details)
kromanova edited edge metadata.

I re-implemented the fix in the FE, updated the test.

One question, if you're up for it - there is actually an implicit UsingDirectiveDecl in the AST (as seen here:

$ cat ns.cpp
namespace {
int i;
}
blaikie@blaikie-linux:/tmp/dbginfo$ clang++-tot -Xclang -ast-dump ns.cpp -fsyntax-only
TranslationUnitDecl 0x69a9170 <<invalid sloc>> <invalid sloc>

-TypedefDecl 0x69a96b0 <<invalid sloc>> <invalid sloc> implicit int128_t 'int128'
-TypedefDecl 0x69a9710 <<invalid sloc>> <invalid sloc> implicit uint128_t 'unsigned int128'
-TypedefDecl 0x69a9b10 <<invalid sloc>> <invalid sloc> implicit builtin_va_list 'va_list_tag [1]'
-NamespaceDecl 0x69a9b60 <ns.cpp:1:1, line:3:1> line:1:11
`-VarDecl 0x69a9c20 <line:2:1, col:5> col:5 i 'int'

`-UsingDirectiveDecl 0x69a9bc0 <line:1:11, <invalid sloc>> <invalid sloc> implicit Namespace 0x69a9b60 ''

It's possible we could figure out why that decl is being skipped normally? & avoid skipping it/filter it out later for non-ps4 while leaving it in for ps4, rather than synthesizing the extra imported module from scratch in the debug info code gen?

I took a glance & didn't immediately see why we weren't already visiting this implicit decl - I guess something is filtering out implicit decls somewhere?

Hi David,
Sorry for the delay answering your question about AST.

One question, if you're up for it - there is actually an implicit UsingDirectiveDecl in the AST

..

It's possible we could figure out why that decl is being skipped normally? & avoid skipping it/filter it out later for non-ps4 while leaving it in for ps4, rather than synthesizing the extra
imported module from scratch in the debug info code gen?
I took a glance & didn't immediately see why we weren't already visiting this implicit decl - I guess something is filtering out implicit decls somewhere?

Implicit UsingDirectiveDecl is generated by Sema::ActOnStartNamespaceDef() when it’s given an anonymous namespace.
The UsingDirectedDecl is added to the Parent (top level) which is why you see it in AST dump.

Above function is called from ParseNamespace(), which is called by ParseDeclaration(). ParseDeclaration() returns DeclGroup.
The problem is that group is populated with a SingleDecl that ParseNamespace returns ([anonymous] NamespaceDecl).

ParseDeclaration doesn’t know anything about the UsingNamespaceDecl that ParseNamespace implicitly generated
and added to the parent.. This implicit declaration is not added to the DeclGroup and thus you don’t generate
debug node for it.

If we want ParseNamespace to generate both NamespaceDecl AND an implicit UsingNamespaceDecl
then the following piece of code:

case tok::kw_namespace:
  ProhibitAttributes(attrs);
  SingleDecl = ParseNamespace(Context, DeclEnd);
  break;

should return not the SingleDecl (converted to DeclGroup), but a DeclGroup containing both NamespaceDecl AND UsingNamespaceDecl.

I think that this fix (conditional for PS4 only) will be more complicated and more invasive solution than FE solution that I implemented before.

I saw that you already accepted my latest FE patch. Please confirm that you didn't change your mind about how this bug should get fixed after your saw this explanation about AST.

If everything is OK, I will rebase and commit my latest FE patch.
Thanks for reviewing!

From: David Blaikie [mailto:dblaikie@gmail.com]
Sent: Wednesday, May 13, 2015 11:06 AM
To: reviews+D7895+public+7827be49c0b04087@reviews.llvm.org
Cc: Romanova, Katya; Eric Christopher; Frédéric Riss; Duncan P. N. Exon Smith; Robinson, Paul; Adrian Prantl; llvm-commits@cs.uiuc.edu
Subject: Re: [PATCH] Anonymous namespaces are missing import DW_TAG_imported_module.

kromanova updated this revision to Diff 32354.Aug 17 2015, 4:54 PM

Hi David,

Sorry for the delay. As per your request, I have changed to patch to generate (hopefully) correct DWARF info for nested anonymous namespace. Currently, clang generates DW_TAG_imported_module for inner (nested) anonymous NS, but not for outer one. So, the situation was somewhat asymmetric.

I have changed clang not to generate DW_TAG_imported_module tag for all the platforms, except PS4 (where we always want to generate DW_TAG_imported_module tags for anonymous NS).

ParseNamespace now returns DeclGroup containing both NamespaceDecl AND UsingNamespaceDecl (unfortunately, I couldn't think of a better way to plumb UsingNamespaceDecl to the top level, so had to change the return type for ParseNamespace).

I added a testcase to check that no DW_TAG_imported_module are generated for nested anonymous namespaces for all the platforms except than PS4 (where we expect to generate two DW_TAG_imported_modules, one for inner, another for outer NS).

I don't think this should be PS4-special. I know I was more wishy-washy about it before, but really DWARF-the-standard tries to be language-neutral. Emitting an explicit import matches the intent of DWARF, and Clang should always do it.

DWARF describes a mapping from source to object. It tries to use language-neutral mechanisms to do this. "Namespace" is not a C++-specific notion. "Importing" a name or set of names from one scope to another is not a C++-specific notion. Emitting an artificial import seems like a perfectly natural way to translate the C++ language-specific rule about anonymous namespaces into a language-neutral DWARF description.

This is not about how to let a user identify an entity available within a scope; this is about what entities are available within the scope in the first place. The artificial import gives you that. Failing to provide the import imposes a *requirement* on the client to understand language-specific scope-munging rules, in order for the client to even know what entities are available. DWARF tries hard not to impose that kind of requirement. (DWARF does assume that inner scopes can see all entities in outer scopes, which is how most but not all languages work. COBOL's data-visibility rules are really arcane.)

Regarding ADL, it is not a DWARF-imposed requirement that every debug-info client understand all the C++-defined shorthand methods for identifying an entity. While could be very user-friendly of the client to permit users to type in C++-like syntax to name things, and disambiguate them for you, that depends on the client's UI and at most is a quality-of-implementation issue for the client, not something assumed or imposed (and certainly not required) by DWARF.

Regarding calling conventions, a lot of the relevant information is actually explicit in the DWARF, and (without having thought about it much) I expect what's implicit would be platform-dependent not language-dependent. Clients *are* expected to understand the target platform.

To recap: An artificial import of a C++ anonymous namespace conforms to DWARF's intent, and having it be target-dependent is silly.
--paulr

From: David Blaikie [mailto:dblaikie@gmail.com]
Sent: Monday, August 17, 2015 6:16 PM
To: reviews+D7895+public+7827be49c0b04087@reviews.llvm.org; Robinson, Paul
Cc: Romanova, Katya; Eric Christopher; Frédéric Riss; Duncan P. N. Exon Smith; llvm-commits
Subject: Re: [PATCH] D7895: Anonymous namespaces are missing import DW_TAG_imported_module.

Hi David,

Sorry for the delay. As per your request, I have changed to patch to generate (hopefully) correct DWARF info for nested anonymous namespace. Currently, clang generates DW_TAG_imported_module for inner (nested) anonymous NS, but not for outer one. So, the situation was somewhat asymmetric.

I have changed clang not to generate DW_TAG_imported_module tag for all the platforms, except PS4 (where we always want to generate DW_TAG_imported_module tags for anonymous NS).

ParseNamespace now returns DeclGroup containing both NamespaceDecl AND UsingNamespaceDecl (unfortunately, I couldn't think of a better way to plumb UsingNamespaceDecl to the top level, so had to change the return type for ParseNamespace).

This is a somewhat more involved change which we might want to check with Richard or other frontend people if it's the right way to go.

It's possible that the opposite way might make sense - we could make the current situation consistent by ignoring implicit using decls. Then we could add the functionality you need by just manually creating an imported module whenever we see an anonymous namespace. Would that make sense?

(I'd do this as two patches - first make the current world consistent (no imported modules for any anonymous namespaces) then add the special case for anonymous namespaces on PS4 that the get this explicit imported namespace thing - just as a Debug-only thing (though I suppose it could be interesting to know if there are other sources of implicit using directives, as PS4 might want those too - in which case your current approach might be the one to aim for (& even if that's not the case, there's a certain elegance to it, for sure - "PS4 gets DWARF for implicit using directives" rather than special casing anonymous namespaces directl, etc))

I added a testcase to check that no DW_TAG_imported_module are generated for nested anonymous namespaces for all the platforms except than PS4 (where we expect to generate two DW_TAG_imported_modules, one for inner, another for outer NS).

David, do you mind if I open a new review in cfe-commit for this bug (this review is still is under llvm-commit thread, so cfe people don't see it) and ask Richard Smith's opinion? Hopefully, it will be less confusing than CC-ing Richard into the current phabricator review.

Sure - feel free to quote me on whatever bits seem relevant.

kromanova abandoned this revision.Jan 3 2016, 4:44 AM

This review was opened in llvm-commits, since the first patch that I submitted was in the backend.
The patch eventually was implemented in the frontend, so I opened a new review in cfe-dev to serve this purpose:
http://reviews.llvm.org/D12624 (closed by commit rL255281).

I'm abandoning this backend review D7895.