[llvm] r263379 - Fixed DIBuilder to verify that same imported entity will not be added twice to the "imports" list of the DICompileUnit.
David Blaikie via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 18 09:01:14 PDT 2016
I think I raised another option early in the review that was never answered
http://reviews.llvm.org/D17884
"I think the fix could probably happen at a higher level to avoid the linear
search. Perhaps we could get the canonical decl for a using decl - that way
a redecl wouldn't be a 'new' thing? I suppose we'd still need a map of
them, that perhaps we don't have already."
But yeah, generally I think it'd be better to move this up rather than down
- but I'm not too fussed/open to either.
On Mon, Apr 18, 2016 at 7:15 AM, Duncan P. N. Exon Smith via llvm-commits <
llvm-commits at lists.llvm.org> wrote:
>
> > 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
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160418/b0742b94/attachment.html>
More information about the llvm-commits
mailing list