[llvm] r263379 - Fixed DIBuilder to verify that same imported entity will not be added twice to the "imports" list of the DICompileUnit.

[Duncan P. N. Exon Smith via llvm-commits]

> On 2016-Mar-13, at 04:11, Amjad Aboud via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> Author: aaboud
> Date: Sun Mar 13 06:11:39 2016
> New Revision: 263379
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=263379&view=rev
> Log:
> Fixed DIBuilder to verify that same imported entity will not be added twice to the "imports" list of the DICompileUnit.

I don't like that this peeks into the LLVMContext, and it doesn't seem
like it's correct.  See below.  (Sorry for missing this when it went
by.)

> 
> Differential Revision: http://reviews.llvm.org/D17884
> 
> Modified:
>    llvm/trunk/lib/IR/DIBuilder.cpp
>    llvm/trunk/unittests/IR/IRBuilderTest.cpp
> 
> Modified: llvm/trunk/lib/IR/DIBuilder.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/IR/DIBuilder.cpp?rev=263379&r1=263378&r2=263379&view=diff
> ==============================================================================
> --- llvm/trunk/lib/IR/DIBuilder.cpp (original)
> +++ llvm/trunk/lib/IR/DIBuilder.cpp Sun Mar 13 06:11:39 2016
> @@ -19,6 +19,7 @@
> #include "llvm/IR/Module.h"
> #include "llvm/Support/Debug.h"
> #include "llvm/Support/Dwarf.h"
> +#include "LLVMContextImpl.h"
> 
> using namespace llvm;
> using namespace llvm::dwarf;
> @@ -168,8 +169,12 @@ 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);

What if `M` isn't a new node because some other llvm::Module (or
even a different llvm::DIBuilder instance in the same llvm::Module)
has added a structurally equivalent DIImportedEntity?  You would still
need it in AllImportedModules, right?  But this logic won't do that.

> -  AllImportedModules.emplace_back(M);
> +  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);

I think you just want to change AllImportedModules to a SetVector so
that they're uniqued on insert.

>   return M;
> }
> 
> 
> Modified: llvm/trunk/unittests/IR/IRBuilderTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/IR/IRBuilderTest.cpp?rev=263379&r1=263378&r2=263379&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/IR/IRBuilderTest.cpp (original)
> +++ llvm/trunk/unittests/IR/IRBuilderTest.cpp Sun Mar 13 06:11:39 2016
> @@ -418,4 +418,19 @@ TEST_F(IRBuilderTest, DebugLoc) {
> 
>   DIB.finalize();
> }
> +
> +TEST_F(IRBuilderTest, DIImportedEntity) {
> +  IRBuilder<> Builder(BB);
> +  DIBuilder DIB(*M);
> +  auto File = DIB.createFile("F.CBL", "/");
> +  auto CU = DIB.createCompileUnit(dwarf::DW_LANG_Cobol74, "F.CBL", "/",
> +    "llvm-cobol74", true, "", 0);
> +  auto IE1 = DIB.createImportedDeclaration(CU, nullptr, 1);
> +  auto IE2 = DIB.createImportedDeclaration(CU, nullptr, 1);
> +  auto IE3 = DIB.createImportedModule(CU, (DIImportedEntity*)nullptr, 2);
> +  auto IE4 = DIB.createImportedModule(CU, (DIImportedEntity*)nullptr, 2);
> +  DIB.finalize();
> +  EXPECT_TRUE(verifyModule(*M));
> +  EXPECT_TRUE(CU->getImportedEntities().size() == 2);
> +}
> }
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits