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

Aboud, Amjad via llvm-commits llvm-commits at lists.llvm.org
Mon Apr 18 06:12:21 PDT 2016


We thought about this direction by then too, but SetVector does implement the function emplace_back.
If you have an idea on how to make it work with a simple change of the " AllImportedModules" container type, it would be great.

Thanks,
Amjad


> -----Original Message-----
> From: dexonsmith at apple.com [mailto:dexonsmith at apple.com]
> Sent: Monday, April 18, 2016 09:18
> To: Aboud, Amjad <amjad.aboud at intel.com>
> Cc: llvm-commits <llvm-commits at lists.llvm.org>; hintonda at gmail.com; David
> Blaikie <dblaikie at gmail.com>
> Subject: Re: [llvm] r263379 - Fixed DIBuilder to verify that same imported entity
> will not be added twice to the "imports" list of the DICompileUnit.
> 
> 
> > 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?re
> > v=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/IRBuilderT
> > est.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

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.



More information about the llvm-commits mailing list