kromanova created this revision.
kromanova added reviewers: rsmith, dblaikie, echristo, probinson, aprantl.
kromanova added a subscriber: cfe-commits.


I had a review opened for anonymous namespaces bug in llvm-commits (see 
http://reviews.llvm.org/D7895). It was opened in llvm-commits is because the 
first patch that I submitted was in the backend. However it seems that the 
frontend patch will be more appropriate in this case. David Blaikie advised to 
get cfe developers opinion, specifically Richard Smith's.
Rather than just adding cfe-dev to the reviewers list, I thought it will be 
better to open a brand new review.

There was quite a long discussion in llvm-commits list about this bug and 
several re-implementations of a fix, so it gets confusing after a while. 
I will write a short summary of what was discussed in 
http://reviews.llvm.org/D7895

Problem #1:

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

Consider the following testcase. Lack of DW_TAG_imported_module prevents our 
debugger from displaying the value of ‘a’.

#include <stdio.h>

namespace
{

int a = 5;

}

int main()
{

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

return 0;

}


Problem #2:

Another (more general) problem is that while the clang compiler doesn't 
generate DW_TAG_imported_module for the top-level anonymous namespace, it 
generates this tag for nested anonymous namespaces. So, we have an asymmetrical 
situation.

namespace
{
  namespace {
    int a1 = 5;
  }
  int a2 = 7;
}
int *b1 = &a1;
int *b2 = &a2;



DWARF:
 <1><54>: Abbrev Number: 5 (DW_TAG_namespace)
    <55>   DW_AT_decl_file   : 1
    <56>   DW_AT_decl_line   : 2
 <2><57>: Abbrev Number: 5 (DW_TAG_namespace)
    <58>   DW_AT_decl_file   : 1
    <59>   DW_AT_decl_line   : 3
....
 <2><8d>: Abbrev Number: 7 (DW_TAG_imported_module)
    <8e>   DW_AT_import      : <0x57>   [Abbrev Number: 5 (DW_TAG_namespace)]

Note that DW_TAG_import is not generated for the top-level anonymous namespace.



There was a long discussion if we want to generate explicit imports marked by 
DW_AT_artificial attribute for anonymous namespaces in Clang or not. Both GCC 
and ICC compilers generate explicit imports in this case. However, it's an 
arguable gray area between the compiler and the debugger.  The final decision 
was *not* to generate DW_TAG_imported_module for all the platforms except PS4. 
On PS4 we definitely want it, since our debugger doesn't import the anonymous 
namespace automatically.

There were several different patches for this fix. For this review, I'm 
providing the latest FE patch only. It takes care of both problem #1 and 
problem #2.
See http://reviews.llvm.org/D7895 for more details/earlier patches.

Richard, could you please give your opinion on how that problem needs to get 
taken care of in the FE. I don't mind re-writing patch based on your comments, 
if necessary.
Thank you!











http://reviews.llvm.org/D12624

Files:
  include/clang/Parse/Parser.h
  include/clang/Sema/Sema.h
  lib/CodeGen/CGDebugInfo.cpp
  lib/Parse/ParseDecl.cpp
  lib/Parse/ParseDeclCXX.cpp
  lib/Sema/SemaDeclCXX.cpp
  test/CodeGenCXX/debug-info-anon-namespace.cpp

Index: test/CodeGenCXX/debug-info-anon-namespace.cpp
===================================================================
--- test/CodeGenCXX/debug-info-anon-namespace.cpp
+++ test/CodeGenCXX/debug-info-anon-namespace.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -emit-llvm -g -triple x86_64-scei-ps4 -O0 %s -o - | FileCheck --check-prefix=PS4 %s
+// RUN: %clang_cc1 -emit-llvm -g -triple x86_64-unknown-linux-gnu -O0 %s -o - | FileCheck --check-prefix=NON-PS4 %s
+
+namespace
+{
+  int a = 5;
+}
+int *b = &a;
+
+namespace
+{
+  namespace {
+    int a1 = 5;
+  }
+  int a2 = 7;
+}
+int *b1 = &a1;
+int *b2 = &a2;
+
+
+// PS4:  [[NS:![0-9]+]] = !DINamespace
+// PS4:  [[NS2:![0-9]+]] = !DINamespace
+// PS4: !DIImportedEntity(tag: DW_TAG_imported_module, scope: !0, entity: [[NS]])
+// PS4: !DIImportedEntity(tag: DW_TAG_imported_module, scope: [[NS]], entity: [[NS2]], line: {{[0-9]+}})
+// NON-PS4-NOT: !DIImportedEntity
+
Index: lib/Sema/SemaDeclCXX.cpp
===================================================================
--- lib/Sema/SemaDeclCXX.cpp
+++ lib/Sema/SemaDeclCXX.cpp
@@ -7185,7 +7185,8 @@
                                    SourceLocation IdentLoc,
                                    IdentifierInfo *II,
                                    SourceLocation LBrace,
-                                   AttributeList *AttrList) {
+                                   AttributeList *AttrList,
+                                   UsingDirectiveDecl *& UD) {
   SourceLocation StartLoc = InlineLoc.isValid() ? InlineLoc : NamespaceLoc;
   // For anonymous namespace, take the location of the left brace.
   SourceLocation Loc = II ? IdentLoc : LBrace;
@@ -7309,14 +7310,13 @@
     // namespace internal linkage.
 
     if (!PrevNS) {
-      UsingDirectiveDecl* UD
-        = UsingDirectiveDecl::Create(Context, Parent,
-                                     /* 'using' */ LBrace,
-                                     /* 'namespace' */ SourceLocation(),
-                                     /* qualifier */ NestedNameSpecifierLoc(),
-                                     /* identifier */ SourceLocation(),
-                                     Namespc,
-                                     /* Ancestor */ Parent);
+      UD = UsingDirectiveDecl::Create(Context, Parent,
+                                      /* 'using' */ LBrace,
+                                      /* 'namespace' */ SourceLocation(),
+                                      /* qualifier */ NestedNameSpecifierLoc(),
+                                      /* identifier */ SourceLocation(),
+                                      Namespc,
+                                      /* Ancestor */ Parent);
       UD->setImplicit();
       Parent->addDecl(UD);
     }
Index: lib/Parse/ParseDeclCXX.cpp
===================================================================
--- lib/Parse/ParseDeclCXX.cpp
+++ lib/Parse/ParseDeclCXX.cpp
@@ -54,9 +54,9 @@
 ///       namespace-alias-definition:  [C++ 7.3.2: namespace.alias]
 ///         'namespace' identifier '=' qualified-namespace-specifier ';'
 ///
-Decl *Parser::ParseNamespace(unsigned Context,
-                             SourceLocation &DeclEnd,
-                             SourceLocation InlineLoc) {
+Parser::DeclGroupPtrTy Parser::ParseNamespace(unsigned Context,
+                                              SourceLocation &DeclEnd,
+                                              SourceLocation InlineLoc) {
   assert(Tok.is(tok::kw_namespace) && "Not a namespace!");
   SourceLocation NamespaceLoc = ConsumeToken();  // eat the 'namespace'.
   ObjCDeclContextSwitch ObjCDC(*this);
@@ -64,7 +64,7 @@
   if (Tok.is(tok::code_completion)) {
     Actions.CodeCompleteNamespaceDecl(getCurScope());
     cutOffParsing();
-    return nullptr;
+    return DeclGroupPtrTy();;
   }
 
   SourceLocation IdentLoc;
@@ -108,7 +108,7 @@
       Diag(Tok, diag::err_expected) << tok::identifier;
       // Skip to end of the definition and eat the ';'.
       SkipUntil(tok::semi);
-      return nullptr;
+      return DeclGroupPtrTy();
     }
     if (attrLoc.isValid())
       Diag(attrLoc, diag::err_unexpected_namespace_attributes_alias);
@@ -115,8 +115,9 @@
     if (InlineLoc.isValid())
       Diag(InlineLoc, diag::err_inline_namespace_alias)
           << FixItHint::CreateRemoval(InlineLoc);
-    return ParseNamespaceAlias(NamespaceLoc, IdentLoc, Ident, DeclEnd);
-  }
+    Decl *NSAlias = ParseNamespaceAlias(NamespaceLoc, IdentLoc, Ident, DeclEnd);
+    return Actions.ConvertDeclToDeclGroup(NSAlias);
+}
 
 
   BalancedDelimiterTracker T(*this, tok::l_brace);
@@ -125,7 +126,7 @@
       Diag(Tok, diag::err_expected) << tok::l_brace;
     else
       Diag(Tok, diag::err_expected_either) << tok::identifier << tok::l_brace;
-    return nullptr;
+    return DeclGroupPtrTy();
   }
 
   if (getCurScope()->isClassScope() || getCurScope()->isTemplateParamScope() || 
@@ -133,7 +134,7 @@
       getCurScope()->getFnParent()) {
     Diag(T.getOpenLocation(), diag::err_namespace_nonnamespace_scope);
     SkipUntil(tok::r_brace);
-    return nullptr;
+    return DeclGroupPtrTy();
   }
 
   if (ExtraIdent.empty()) {
@@ -180,10 +181,11 @@
   // Enter a scope for the namespace.
   ParseScope NamespaceScope(this, Scope::DeclScope);
 
+  UsingDirectiveDecl *ImplicitUsingDirectiveDecl = nullptr;
   Decl *NamespcDecl =
     Actions.ActOnStartNamespaceDef(getCurScope(), InlineLoc, NamespaceLoc,
                                    IdentLoc, Ident, T.getOpenLocation(), 
-                                   attrs.getList());
+                                   attrs.getList(), ImplicitUsingDirectiveDecl);
 
   PrettyDeclStackTraceEntry CrashInfo(Actions, NamespcDecl, NamespaceLoc,
                                       "parsing namespace");
@@ -198,8 +200,10 @@
 
   DeclEnd = T.getCloseLocation();
   Actions.ActOnFinishNamespaceDef(NamespcDecl, DeclEnd);
-
-  return NamespcDecl;
+  
+ // Return NamespaceDecl + ImportDecl here.
+  return Actions.ConvertDeclToDeclGroup(NamespcDecl, 
+                                        ImplicitUsingDirectiveDecl);
 }
 
 /// ParseInnerNamespace - Parse the contents of a namespace.
@@ -228,17 +232,17 @@
   // FIXME: Preserve the source information through to the AST rather than
   // desugaring it here.
   ParseScope NamespaceScope(this, Scope::DeclScope);
+  UsingDirectiveDecl *ImplicitUsingDirectiveDecl = NULL;
   Decl *NamespcDecl =
     Actions.ActOnStartNamespaceDef(getCurScope(), SourceLocation(),
                                    NamespaceLoc[index], IdentLoc[index],
                                    Ident[index], Tracker.getOpenLocation(), 
-                                   attrs.getList());
+                                   attrs.getList(), ImplicitUsingDirectiveDecl);
 
   ParseInnerNamespace(IdentLoc, Ident, NamespaceLoc, ++index, InlineLoc,
                       attrs, Tracker);
 
   NamespaceScope.Exit();
-
   Actions.ActOnFinishNamespaceDef(NamespcDecl, Tracker.getCloseLocation());
 }
 
Index: lib/Parse/ParseDecl.cpp
===================================================================
--- lib/Parse/ParseDecl.cpp
+++ lib/Parse/ParseDecl.cpp
@@ -1468,15 +1468,13 @@
     if (getLangOpts().CPlusPlus && NextToken().is(tok::kw_namespace)) {
       ProhibitAttributes(attrs);
       SourceLocation InlineLoc = ConsumeToken();
-      SingleDecl = ParseNamespace(Context, DeclEnd, InlineLoc);
-      break;
+      return ParseNamespace(Context, DeclEnd, InlineLoc);
     }
     return ParseSimpleDeclaration(Context, DeclEnd, attrs,
                                   true);
   case tok::kw_namespace:
     ProhibitAttributes(attrs);
-    SingleDecl = ParseNamespace(Context, DeclEnd);
-    break;
+    return ParseNamespace(Context, DeclEnd);
   case tok::kw_using:
     SingleDecl = ParseUsingDirectiveOrDeclaration(Context, ParsedTemplateInfo(),
                                                   DeclEnd, attrs, &OwnedType);
Index: lib/CodeGen/CGDebugInfo.cpp
===================================================================
--- lib/CodeGen/CGDebugInfo.cpp
+++ lib/CodeGen/CGDebugInfo.cpp
@@ -3259,10 +3259,14 @@
 void CGDebugInfo::EmitUsingDirective(const UsingDirectiveDecl &UD) {
   if (CGM.getCodeGenOpts().getDebugInfo() < CodeGenOptions::LimitedDebugInfo)
     return;
-  DBuilder.createImportedModule(
-      getCurrentContextDescriptor(cast<Decl>(UD.getDeclContext())),
-      getOrCreateNameSpace(UD.getNominatedNamespace()),
-      getLineNumber(UD.getLocation()));
+  const NamespaceDecl *NSDecl = UD.getNominatedNamespace();
+  if (!NSDecl->isAnonymousNamespace() || 
+      CGM.getTarget().getTriple().isPS4CPU()) { 
+    DBuilder.createImportedModule(
+        getCurrentContextDescriptor(cast<Decl>(UD.getDeclContext())),
+        getOrCreateNameSpace(NSDecl),
+        getLineNumber(UD.getLocation()));
+  }
 }
 
 void CGDebugInfo::EmitUsingDecl(const UsingDecl &UD) {
Index: include/clang/Sema/Sema.h
===================================================================
--- include/clang/Sema/Sema.h
+++ include/clang/Sema/Sema.h
@@ -4054,7 +4054,8 @@
                                SourceLocation IdentLoc,
                                IdentifierInfo *Ident,
                                SourceLocation LBrace,
-                               AttributeList *AttrList);
+                               AttributeList *AttrList,
+                               UsingDirectiveDecl * &UsingDecl);
   void ActOnFinishNamespaceDef(Decl *Dcl, SourceLocation RBrace);
 
   NamespaceDecl *getStdNamespace() const;
Index: include/clang/Parse/Parser.h
===================================================================
--- include/clang/Parse/Parser.h
+++ include/clang/Parse/Parser.h
@@ -2335,8 +2335,8 @@
 
   void DiagnoseUnexpectedNamespace(NamedDecl *Context);
 
-  Decl *ParseNamespace(unsigned Context, SourceLocation &DeclEnd,
-                       SourceLocation InlineLoc = SourceLocation());
+  DeclGroupPtrTy ParseNamespace(unsigned Context, SourceLocation &DeclEnd,
+                                SourceLocation InlineLoc = SourceLocation());
   void ParseInnerNamespace(std::vector<SourceLocation>& IdentLoc,
                            std::vector<IdentifierInfo*>& Ident,
                            std::vector<SourceLocation>& NamespaceLoc,
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to