Page MenuHomePhabricator

Disallow duplication of imported entities (improved implementation)
AcceptedPublic

Authored by aaboud on Apr 24 2016, 6:47 AM.

Details

Summary

Following Duncan suggestion of improving code to disallow duplication of imported entities from commit: http://reviews.llvm.org/rL263379
By simply filter the AllImportedModules list at DIBuilder::finalize function.

Note: LIT test was committed in revision 263379.

Diff Detail

Event Timeline

aaboud updated this revision to Diff 54802.Apr 24 2016, 6:47 AM
aaboud retitled this revision from to Disallow duplication of imported entities (improved implementation).
aaboud updated this object.
aaboud added reviewers: dexonsmith, dblaikie, aprantl.
aaboud added a subscriber: llvm-commits.
aprantl added inline comments.Apr 24 2016, 2:26 PM
lib/IR/DIBuilder.cpp
131

Maybe assert that CUNode->getImportedEntities() is empty?

178

This is more a general question: Is there a difference between emplace_back for a *pointer* element and a regular push_back?

Thanks Adrian for the comments, please see answers below.

lib/IR/DIBuilder.cpp
131

I do not mind adding assertion, but how this is different than all the other replace function, e.g. the one at line 120.
Should we add assertion for all that the list was empty before replacing it with the actual values?

178

push_back assumes you are adding element of type T, where T in our case is "TrackingMDNodeRef".
emplace_back will take the element(s) as arguments that can be passed to the T constructor during allocation.

I guess we could call the push_back like this:
AllImportedModules.emplace_back(TrackingMDNodeRef(M));

At least this is my understanding.
Also notice that I am just reverting this part from commit 263379.

aprantl accepted this revision.Apr 25 2016, 9:29 AM
aprantl edited edge metadata.

One last thing: If there isn't one already, there should be a unit test for the uniquing.
LGTM with these points addressed. Thanks!

lib/IR/DIBuilder.cpp
131

I think the answer is yes. DIBuilder::finalize() makes this assumption here and will happily replace the contents of all of these arrays, so we should take the opportunity to guard against misuse of the interface by asserting that they are empty before replacing them.

178

That makes sense. I didn't look at the type AllImportedModules.

This revision is now accepted and ready to land.Apr 25 2016, 9:29 AM

What’s the benefit to duplicating this code in every frontend? (Ok, having the code in DIBuilder is imposing extra work for frontends where duplicated entries are impossible.)

I'm still vaguely thinking we should be doing this in the frontend instead.

Could you remind me what problems happen if there are duplicates? And why
it was hard to detect duplicates in the frontend? (why we even ever visited
the same using decl more than once?)

I am not much familiar with the AST part, but I assume there is reuse of previous generated DCL, even if it has same data.

Here is a small example that leads into duplication in AST DCLs.
You may run the following command line:

clang -cc1 -ast-dump -o - -O0 TestIM.cpp

TestIM.cpp

#include "TestIM.h"
#include "TestIM.h"

TestIM.h

namespace X {
  typedef int MyINT_t;
}

namespace Y {
  using X::MyINT_t;
}
aaboud added a subscriber: aaboud.Apr 26 2016, 1:01 AM

You cannot simply skip a redeclaration unless it is declared at same location.

For example:

namespace X { int a; }
// some other code
namespace X { int b; }

You cannot eliminate the second namespace, can you?

void foo () { using X:a; }
void bar () { using X:a; }

Also here, you cannot eliminate the second using-decl can you?

Looking into the code, it is not simple to introduce these checks, as there is no one place that will affect all Decls, and we will need to handle each Decl separately.
Can you think on a place where we can add this in AST?

Thanks,
Amjad

From: David Blaikie [mailto:dblaikie@gmail.com]
Sent: Tuesday, April 26, 2016 03:12
To: reviews+D19468+public+d141c7248d054ca3@reviews.llvm.org; Aboud, Amjad <amjad.aboud@intel.com>
Cc: Duncan P. N. Exon Smith <dexonsmith@apple.com>; Adrian Prantl <aprantl@apple.com>; llvm-commits <llvm-commits@lists.llvm.org>
Subject: Re: [PATCH] D19468: Disallow duplication of imported entities (improved implementation)

dblaikie edited edge metadata.Apr 27 2016, 11:21 AM

Agreed with Adrian - a unit test would be good.

lib/IR/DIBuilder.cpp
125–129

This formatting looks weird - is that what clang-format produced?