[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-Apr-18, at 06:12, Aboud, Amjad <amjad.aboud at intel.com> wrote:
> 
> 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.

Ah, right; TrackingMDNodeRef isn't stable in value so it doesn't
belong in a DenseSet, etc.

Did you consider filtering them in DIBuilder::finalize?
--
  SmallPtrSet<const Metadata *, 16> AlreadyImported;
  std::remove_if(AllImportedModules.begin(), AllImportedModules.end(),
      [&AlreadyImported](const TrackingMDNodeRef &Ref) {
        return !AlreadyImported.insert(Ref.get()).second;
      });
--

> 
> 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.
>