Page MenuHomePhabricator

[ASTImporter] Fix name conflict handling with different strategies
ClosedPublic

Authored by martong on Mar 22 2019, 5:47 AM.

Details

Summary

There are numorous flaws about the name conflict handling, this patch
attempts fixes them. Changes in details:

  • HandleNameConflict return with a false DeclarationName

Hitherto we effectively never returned with a NameConflict error, even
if the preceding StructuralMatch indicated a conflict.
Because we just simply returned with the parameter Name in
HandleNameConflict and that name is almost always true when converted to
bool.

  • Add tests which indicate wrong NameConflict handling
  • Add to ConflictingDecls only if decl kind is different

Note, we might not indicate an ODR error when there is an existing record decl
and a enum is imported with same name. But there are other cases. E.g. think
about the case when we import a FunctionTemplateDecl with name f and we found a
simple FunctionDecl with name f. They overload. Or in case of a
ClassTemplateDecl and CXXRecordDecl, the CXXRecordDecl could be the 'templated'
class, so it would be false to report error. So I think we should report a
name conflict error only when we are 100% sure of that. That is why I think it
should be a general pattern to report the error only if the kind is the same.

  • Fix failing ctu test with EnumConstandDecl

In ctu-main.c we have the enum class 'A' which brings in the enum
constant 'x' with value 0 into the global namespace.
In ctu-other.c we had the enum class 'B' which brought in the same name
('x') as an enum constant but with a different enum value (42). This is clearly
an ODR violation in the global namespace. The solution was to rename the
second enum constant.

  • Introduce ODR handling strategies

Event Timeline

martong created this revision.Mar 22 2019, 5:47 AM

Hi Gabor,

Thank you for addressing the problem!

lib/AST/ASTImporter.cpp
2256 ↗(On Diff #191864)

if body is surrounded by braces, so it's better to surround else too.

2260 ↗(On Diff #191864)

Do we push the same decl twice?

8532 ↗(On Diff #191864)

Empty DeclarationName can be valid sometimes. Should we return ErrorOr<DeclarationName> instead? This can also simplify caller code a bit.

martong marked 5 inline comments as done.Mar 26 2019, 5:33 AM
martong added inline comments.
lib/AST/ASTImporter.cpp
2260 ↗(On Diff #191864)

Uhh, thanks, that is rebase error I've made.

8532 ↗(On Diff #191864)

That's a good idea, thanks.
Though, I'd use Expected<T> rather, that would be more consistent with the rest of the code.

martong updated this revision to Diff 192263.Mar 26 2019, 5:34 AM
martong marked 2 inline comments as done.
  • Add braces around else
  • Remove falsely duplicated push_back
  • Use Expected<T> in HandleNameConflict

Thanks for the fixes! I'd like to clarify one moment, however.

lib/AST/ASTImporter.cpp
2154 ↗(On Diff #192263)

Should we write else Name = *NameOrError?

martong marked 2 inline comments as done.Apr 2 2019, 2:11 AM
martong added inline comments.
lib/AST/ASTImporter.cpp
2154 ↗(On Diff #192263)

Atm, we implement the simplest strategy for name conflict handling: we just return with the error.
However, on a long term we should use the returned name indeed. I mean when we really do implement a renaming strategy via HandleNameConflict.

Also, for that we'd have to double check that the Name is indeed used when we create the AST node. So I'd rather leave this else branch up to that point. Hopefully, by that time we'll have unittests which would exercise this else branch, now we just don't have any.

martong updated this revision to Diff 194527.Apr 10 2019, 8:37 AM
martong marked an inline comment as done.
  • Put back the call to GetOriginalDecl
martong edited the summary of this revision. (Show Details)Apr 10 2019, 8:38 AM
a_sidorin accepted this revision.May 4 2019, 2:56 AM

LGTM, thanks!

This revision is now accepted and ready to land.May 4 2019, 2:56 AM
martong updated this revision to Diff 211312.Jul 23 2019, 9:01 AM
  • Rebase to master

First round of review.

clang/lib/AST/ASTImporter.cpp
2699

Name -> SearchName

clang/unittests/AST/ASTImporterTest.cpp
2392

What about tests for name conflicts for:

NamespaceDecl
TypedefNameDecl
TypeAliasTemplateDecl
EnumConstantDecl
RecordDecl
VarDecl

Who were also modified above.

Just wanted to see if you were planning on landing this soon, the fix Name -> SearchName is probably an important one since we have seen several issues w/ bugs like that but I really would like to see more tests. Are you having issues coming up w/ tests?

martong marked an inline comment as done.Aug 16 2019, 3:23 AM

Just wanted to see if you were planning on landing this soon, the fix Name -> SearchName is probably an important one since we have seen several issues w/ bugs like that but I really would like to see more tests. Are you having issues coming up w/ tests?

Hey Shafik, I was busy lately with some non-ASTImporter related tasks, but just recently I could drive myself back to it.
I am planning to add the test cases in the following work days, but the latest by the end of the next week.

martong updated this revision to Diff 215579.Aug 16 2019, 6:18 AM
  • Name -> SearchName
  • Add tests for conflicting declarations
martong marked 2 inline comments as done.Aug 16 2019, 6:20 AM
martong added inline comments.
clang/unittests/AST/ASTImporterTest.cpp
2392

I added several new tests with a new test suite ConflictingDeclsTest, they cover all the modifications in ASTImporter.cpp except for VarTemplateDecls.
In case of VarTemplateDecls we don't have a proper structural eq check implemented yet, so I disabled that test.

martong marked an inline comment as done.Aug 23 2019, 4:26 AM

@shafik Ping

martong updated this revision to Diff 217135.Aug 26 2019, 6:40 AM
  • Use resulting Name from HandleNameConflict if set
  • Add ODRHandling strategies
  • Refactor tests, to avoid some code repetition
Herald added a project: Restricted Project. · View Herald TranscriptAug 26 2019, 6:40 AM
martong retitled this revision from [ASTImporter] Fix name conflict handling to [ASTImporter] Fix name conflict handling with different strategies.Aug 26 2019, 6:41 AM
martong edited the summary of this revision. (Show Details)

@shafik I have updated the patch with ODR handling strategies as per our discusson.

For the record I copy our discussion here:

On Sat, Aug 24, 2019 at 12:50 AM Shafik Yaghmour <yaghmour.shafik@gmail.com> wrote:

Apologies, I missed this earlier!

On Wed, Aug 21, 2019 at 2:44 AM Gábor Márton <martongabesz@gmail.com> wrote:
>
> Hi Shafik,
>
> > Right now you will end up w/ an arbitrary one of them but we do want to support
> a way to choose between them eventually.
>
> Actually, right now (if the patch is not merged) we end up having both of them in the AST. Some parts of the AST reference the existing definition, while some other parts reference the new definition. Also, the regular lookup will find both definitions.
>
> If the patch is merged then only the first (the existing) definition is kept and an error is reported.
>
> > AFAICT this would prevent such a solution. At least that is how the
> new test case for RecordDecl make it appear
>
> Yes, you will never be able to remove an existing node from the AST, so I don't think an either-or choosing mechanism is feasible. But you may be able to decide whether you want to add the new and conflicting node. And you may be able to use a different name for the new conflicting node. This may help clients to see which node they are using.
> I want to create an additional patch which builds on this patch. Here is the essence of what I'd like to have:
> if (!ConflictingDecls.empty()) {
> Expected<DeclarationName> Resolution = Importer.HandleNameConflict(
> Name, DC, Decl::IDNS_Member, ConflictingDecls.data(),
> ConflictingDecls.size());
> if (Resolution)
> Name = Resolution.get();
> else
> return Resolution.takeError();
> }
> Consider the "else" branch. I'd like to have such an "else" branch everywhere. The point is to use the result of HandleNameConflict (if it is set to something). This way it is possible to create any kind of ODR handling by overwriting HandleNameConflict in the descendant classes.
>
> We identified 3 possible strategies so far:
>
> Conservative. In case of name conflict propagate the error. This should be the default behavior.
> Liberal. In case of name conflict create a new node with the same name and do not propagate the error.
> Rename. In case of name conflict create a new node with a different name (e.g. with a prefix of the TU's name). Do not propagate the error.
>
>
> If we add a new conflicting node beside the exiting one, then some clients of the AST which rely on lookup will be confused. The CTU client does not rely on lookup so that is not really a problem there, but I don't know what this could cause with LLDB. Perhaps the renaming strategy could work there too.
> The Conservative and Liberal strategies are very easy to implement, and I am going to create patches for that if we have consensus.

We know currently we do have cases where we have ODR violations w/
RecordDecl due to use of an opaque struct in the API headers and a
concrete instance internally e.g.:

//opaque
struct A {

char buf[16];

};

//concrete
struct A {

double d;
int64_t x;

};

and we don't want this to become an error.

I think we would at least one the choice of Conservative or Liberal to
be configurable and maybe LLDB would default to Liberal. This would
enable us to keep the status quo and not break existing cases any
worse than they already are.

I would prefer that would be done in this PR since I don't want to be
in a situation where we branch internally and we have this change but
not the follow-up change.

> > I don't see how like you comment says this does not effect CXXRecordDecl
>
> In my comment I do not say that CXXRecordDecls are exceptions from the general way of handling ODR violations.
> The comment is about that we shall not report ODR errors if we are not 100% sure that we face one.
> So we should report an ODR error only if the found Decl and the newly imported Decl have the same kind.
> I.e. both are CXXRecordDecls.
> For example, let's assume we import a CXXRecordDecl and we find an existing ClassTemplateDecl with the very same Name.
> Then we should not report an ODR violation.

Thank you for the clarification, I misunderstood the comment, now it
makes more sense.

> Thanks,
> Gabor
>
>
> On Mon, Aug 19, 2019 at 5:46 PM Shafik Yaghmour <yaghmour.shafik@gmail.com> wrote:
>>
>> Have a nice vacation :-)
>>
>> On Mon, Aug 19, 2019 at 8:40 AM Gábor Márton <martongabesz@gmail.com> wrote:
>> >
>> > Hi Shafik,
>> >
>> > I'll have an answer for you on Wednesday, I'm on vacation until then.
>> >
>> > Thanks,
>> > Gábor
>> >
>> > On Sat, 17 Aug 2019, 04:30 Shafik Yaghmour, <yaghmour.shafik@gmail.com> wrote:
>> >>
>> >> Hello Gábor,
>> >>
>> >> I was looking at;
>> >>
>> >> https://reviews.llvm.org/D59692
>> >>
>> >> again and I appreciate you added the new tests.
>> >>
>> >> I don't know how I missed this before but we would probably like to
>> >> support ODR violations in some reasonable manner. For example we have
>> >> one API that is using an opaque struct for the public API but the
>> >> private API used a non-opaque definition.
>> >>
>> >> I would have to come up with some sort of test case which is hard to
>> >> do, or at least I did not have success last time I tried. Right now
>> >> you will end up w/ an arbitrary one of them but we do want to support
>> >> a way to choose between them eventually.
>> >>
>> >> AFAICT this would prevent such a solution. At least that is how the
>> >> new test case for RecordDecl make it appear and I don't see how like
>> >> you comment says this does not effect CXXRecordDecl.
>> >>
>> >> -Shafik

shafik accepted this revision.Aug 26 2019, 4:11 PM

Other than my two comment this LGTM

clang/lib/AST/ASTImporter.cpp
3453

I am a little concerned about this change. I am wondering if this was previously handled differently as a band-aid for some other issues that only showed up in rare cases when they iterated over all the results but not when they errored out on the first.

Could we make the changes to VisitFieldDecl a separate PR so it is easier to revert later on if we do find this causes a regression we are not currently covering?

clang/unittests/AST/ASTImporterTest.cpp
5615

Since the "Liberal" strategy should be the current status quo, I think it might make sense to have a first PR that just has the *LiberalStrategy tests to verify that indeed this is the current behavior as we expect.

martong marked 4 inline comments as done.Aug 27 2019, 2:27 AM
martong added inline comments.
clang/lib/AST/ASTImporter.cpp
3453

Ok, I reverted these hunks. I am going to create a follow-up patch which will include these changes.

clang/unittests/AST/ASTImporterTest.cpp
5615

Ok, I removed the test suite ConflictingDeclsWithConservativeStrategy. I am going to create a subsequent patch which adds comprehensive testing for both strategies.

martong updated this revision to Diff 217330.Aug 27 2019, 2:27 AM
martong marked 2 inline comments as done.
  • Revert changes in VisitFieldDecl and in VisitIndirectFieldDecl
  • Remove test suite ConflictingDeclsWithConservativeStrategy
martong updated this revision to Diff 217337.Aug 27 2019, 2:50 AM
  • Apply clang-format
This revision was automatically updated to reflect the committed changes.

Jenkins is not green http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/743/
However, the newly failing test is TestTargetCommand.py, which seems to be unrelated.

labath added a subscriber: labath.Aug 27 2019, 6:38 AM

Jenkins is not green http://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/743/
However, the newly failing test is TestTargetCommand.py, which seems to be unrelated.

That ought to be fixed by r370057.