Disallow duplication of imported entities (improved implementation)

[aaboud]

Summary

Following Duncan suggestion of improving code to disallow duplication of imported entities from commit: http://reviews.llvm.org/rL263379
By simply filter the AllImportedModules list at DIBuilder::finalize function.

Note: LIT test was committed in revision 263379.

[aprantl]

lib/IR/DIBuilder.cpp

131

Maybe assert that CUNode->getImportedEntities() is empty?

178

This is more a general question: Is there a difference between emplace_back for a *pointer* element and a regular push_back?

[aaboud]

Thanks Adrian for the comments, please see answers below.

lib/IR/DIBuilder.cpp

131

I do not mind adding assertion, but how this is different than all the other replace function, e.g. the one at line 120.
Should we add assertion for all that the list was empty before replacing it with the actual values?

178

push_back assumes you are adding element of type T, where T in our case is "TrackingMDNodeRef".
emplace_back will take the element(s) as arguments that can be passed to the T constructor during allocation.

I guess we could call the push_back like this:
AllImportedModules.emplace_back(TrackingMDNodeRef(M));

At least this is my understanding.
Also notice that I am just reverting this part from commit 263379.

[aprantl]

One last thing: If there isn't one already, there should be a unit test for the uniquing.
LGTM with these points addressed. Thanks!

lib/IR/DIBuilder.cpp

131

I think the answer is yes. DIBuilder::finalize() makes this assumption here and will happily replace the contents of all of these arrays, so we should take the opportunity to guard against misuse of the interface by asserting that they are empty before replacing them.

178

That makes sense. I didn't look at the type AllImportedModules.

[aprantl]

What’s the benefit to duplicating this code in every frontend? (Ok, having the code in DIBuilder is imposing extra work for frontends where duplicated entries are impossible.)

[llvm-commits]

I'm still vaguely thinking we should be doing this in the frontend instead.

Could you remind me what problems happen if there are duplicates? And why
it was hard to detect duplicates in the frontend? (why we even ever visited
the same using decl more than once?)

• msg-20005-75.txt162 BDownload

[aaboud]

I am not much familiar with the AST part, but I assume there is reuse of previous generated DCL, even if it has same data.

Here is a small example that leads into duplication in AST DCLs.
You may run the following command line:

clang -cc1 -ast-dump -o - -O0 TestIM.cpp

TestIM.cpp

#include "TestIM.h"
#include "TestIM.h"

TestIM.h

namespace X {
  typedef int MyINT_t;
}

namespace Y {
  using X::MyINT_t;
}

[aaboud]

You cannot simply skip a redeclaration unless it is declared at same location.

For example:

namespace X { int a; }
// some other code
namespace X { int b; }

You cannot eliminate the second namespace, can you?

void foo () { using X:a; }
void bar () { using X:a; }

Also here, you cannot eliminate the second using-decl can you?

Looking into the code, it is not simple to introduce these checks, as there is no one place that will affect all Decls, and we will need to handle each Decl separately.
Can you think on a place where we can add this in AST?

Thanks,
Amjad

From: David Blaikie [mailto:[e-mail]]
Sent: Tuesday, April 26, 2016 03:12
To: [e-mail]; Aboud, Amjad <[e-mail]>
Cc: Duncan P. N. Exon Smith <[e-mail]>; Adrian Prantl <[e-mail]>; llvm-commits <[e-mail]>
Subject: Re: [PATCH] D19468: Disallow duplication of imported entities (improved implementation)

[dblaikie]

Agreed with Adrian - a unit test would be good.

lib/IR/DIBuilder.cpp

125–129

This formatting looks weird - is that what clang-format produced?

lib/IR/DIBuilder.cpp

for (auto *SP : SPs)
     resolveVariables(SP);
   for (auto *N : RetainValues)
     if (auto *SP = dyn_cast<DISubprogram>(N))
       resolveVariables(SP);
 
   if (!AllGVs.empty())
     CUNode->replaceGlobalVariables(MDTuple::get(VMContext, AllGVs));
 
-  if (!AllImportedModules.empty())
-    CUNode->replaceImportedEntities(MDTuple::get(
-        VMContext, SmallVector<Metadata *, 16>(AllImportedModules.begin(),
-                                               AllImportedModules.end())));
+  // Create imported entities list without duplications.
+  SmallVector<Metadata *, 16> ImportedModules;
+  SmallPtrSet<Metadata *, 16> AlreadyImported;
+  std::remove_copy_if(AllImportedModules.begin(), AllImportedModules.end(),
+    std::back_inserter(ImportedModules),
+    [&AlreadyImported](TrackingMDNodeRef &Ref) {
+    return !AlreadyImported.insert(Ref.get()).second;
+  });
+  if (!ImportedModules.empty())
+    CUNode->replaceImportedEntities(MDTuple::get(VMContext, ImportedModules));
 
   // Now that all temp nodes have been replaced or deleted, resolve remaining
   // cycles.
   for (const auto &N : UnresolvedNodes)
     if (N && !N->isResolved())
       N->resolveCycles();
   UnresolvedNodes.clear();
 
DICompileUnit *DIBuilder::createCompileUnit(
   trackIfUnresolved(CUNode);
   return CUNode;
 }
 
 static DIImportedEntity *
 createImportedModule(LLVMContext &C, dwarf::Tag Tag, DIScope *Context,
                      Metadata *NS, unsigned Line, StringRef Name,
                      SmallVectorImpl<TrackingMDNodeRef> &AllImportedModules) {
-  unsigned EntitiesCount = C.pImpl->DIImportedEntitys.size();
   auto *M = DIImportedEntity::get(C, Tag, Context, DINodeRef(NS), Line, Name);
-  if (EntitiesCount < C.pImpl->DIImportedEntitys.size())
-    // A new Imported Entity was just added to the context.
-    // Add it to the Imported Modules list.
-    AllImportedModules.emplace_back(M);
   return M;
 }
 
 DIImportedEntity *DIBuilder::createImportedModule(DIScope *Context,
                                                   DINamespace *NS,
                                                   unsigned Line) {
   return ::createImportedModule(VMContext, dwarf::DW_TAG_imported_module,
                                 Context, NS, Line, StringRef(), AllImportedModules);