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

Mon Apr 18 07:15:06 PDT 2016

> On 2016-Apr-18, at 07:09, Duncan P. N. Exon Smith via llvm-commits <llvm-commits at lists.llvm.org> wrote:
> 
> 
>> 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;
>      });
> --

Or just filter them directly into the ops vector:
--
  SmallVector<const Metadata *, 16> Ops;
  SmallPtrSet<const Metadata *, 16> AlreadyImported;
  std::remove_copy_if(AllImportedModules.begin(), AllImportedModules.end(),
                      std::back_inserter(Ops),
      [&AlreadyImported](const TrackingMDNodeRef &Ref) {
        return !AlreadyImported.insert(Ref.get()).second;
      });
  MDTuple *Imports = Ops.empty() ? nullptr : MDTuple::get(Context, Ops);    
--

> 
>> 
>> 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.
>> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits