Call to HandleNameConflict in VisitEnumDecl mistakeningly using Name instead of SearchName

[shafik]

Summary

https://reviews.llvm.org/D51633 added error handling to the ASTNodeImporter::VisitEnumDecl(...) for the conflicting names case. This could lead to erroneous return of an error in that case since we should have been using SearchName. Name may be empty in the case where we find the name via getTypedefNameForAnonDecl(...).

[shafik]

I was not able to come up with a test that would detect this issue using either clang-import-test nor via any of the methods used in ASTImpoterTest.cpp. I created a regression test on the lldb side, which should pass once this is committed:

https://reviews.llvm.org/D59667

[a_sidorin]

Hi Shafik,

Honestly, I was always wondering what does HandleNameConflict actually do. Its implementation in ASTImporter is trivial and I don't see any of its overrides in LLDB code too. Why do we check its result to be non-empty is a question to me as well. And the more I look at it (and the bug you pointed in it), the more I think there is something wrong with it. Maybe it is better to just remove it at all? I hope LLDB developers have more knowledge about it. Shafik, Davide @davide , do you know its actual purpose?

lib/AST/ASTImporter.cpp

2463

If I understand correctly, this will replace Name with SearchName causing an anonymous enum to be imported as a named a few lines below. It doesn't look like a correct behaviour to me.

[martong]

Hi Shafik,

Thank you for looking into this. This is indeed a bug, because whenever a we encounter an unnamed EnumDecl in the "from" context then we return with a nameconflict error.
However, I think we should fix this differently.
First of all, currently HandleNameConflict just returns the parameter Name it received. This may be unnamed in some cases and thus converts to true. So I think HandleNameConflict should be changed that it would return a default initialized DeclarationName; so once we have anything in the ConflictingDecls then we return with the NameConflict error:

DeclarationName ASTImporter::HandleNameConflict(DeclarationName Name,
                                                DeclContext *DC,
                                                unsigned IDNS,
                                                NamedDecl **Decls,
                                                unsigned NumDecls) {
-  return Name;
+  return DeclarationName();
}

And most importantly, I think we should push into the ConflictingDecls only if the structural match indicates a non-match:

      if (auto *FoundEnum = dyn_cast<EnumDecl>(FoundDecl)) {
        if (isStructuralMatch(D, FoundEnum, true))
          return Importer.MapImported(D, FoundEnum);
+       ConflictingDecls.push_back(FoundDecl);
      }
-
-     ConflictingDecls.push_back(FoundDecl);

I am aware of similar errors like this with other AST nodes. We had a patch in our fork to fix that in January (https://github.com/Ericsson/clang/pull/569/files) I am going to prepare a patch from that cause I see now this affects you guys in LLDB too.

[martong]

In D59665#1438911, @a_sidorin wrote:

Hi Shafik,

Honestly, I was always wondering what does HandleNameConflict actually do. Its implementation in ASTImporter is trivial and I don't see any of its overrides in LLDB code too. Why do we check its result to be non-empty is a question to me as well. And the more I look at it (and the bug you pointed in it), the more I think there is something wrong with it. Maybe it is better to just remove it at all? I hope LLDB developers have more knowledge about it. Shafik, Davide @davide , do you know its actual purpose?

As to my best knowlege, HandleNameConflict is not overridden in LLDB (@davide , @shafik please correct my if I am wrong).
However, I would not remove it, I'd rather fix the problems we have around it.
I think via HandleNameConflict we could implement different strategies about how to handle ODR errors and perhaps that was the original intention to use it for. With CTU (and probably with LLDB too) this could be really helpful: Recently I have discovered that during the CTU analysis of Xerces there are real ODR errors on a typedef, but it would be nice if we could just keep on going with the inlining of a certain function. So, I see three possible different strategies to handle name conflicts:

1. Conservative. In case of name conflict propagate the error. (The function will not be inlined because of a typedef ODR error.)
2. Liberal. In case of name conflict create a new node with the same name and do not propagate the error. (This may give erroneous results if a lookup is performed, but most SA checkers does not do lookup)
3. 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. (This would result the most sane AST, but this is perhaps the most difficult to implement, plus a mapping between old and new names has to be given.)

[martong]

I am aware of similar errors like this with other AST nodes. We had a patch in our fork to fix that in January (https://github.com/Ericsson/clang/pull/569/files) I am going to prepare a patch from that cause I see now this affects you guys in LLDB too.

Just created that patch:
https://reviews.llvm.org/D59692

Unfortunately, this depends on other patches which are already in the queue, please check the stack.

[shafik]

In D59665#1439070, @martong wrote:

Hi Shafik,

Thank you for looking into this. This is indeed a bug, because whenever a we encounter an unnamed EnumDecl in the "from" context then we return with a nameconflict error.
However, I think we should fix this differently.
First of all, currently HandleNameConflict just returns the parameter Name it received. This may be unnamed in some cases and thus converts to true. So I think HandleNameConflict should be changed that it would return a default initialized DeclarationName; so once we have anything in the ConflictingDecls then we return with the NameConflict error:

DeclarationName ASTImporter::HandleNameConflict(DeclarationName Name,
                                                DeclContext *DC,
                                                unsigned IDNS,
                                                NamedDecl **Decls,
                                                unsigned NumDecls) {
-  return Name;
+  return DeclarationName();
}

And most importantly, I think we should push into the ConflictingDecls only if the structural match indicates a non-match:

      if (auto *FoundEnum = dyn_cast<EnumDecl>(FoundDecl)) {
        if (isStructuralMatch(D, FoundEnum, true))
          return Importer.MapImported(D, FoundEnum);
+       ConflictingDecls.push_back(FoundDecl);
      }
-
-     ConflictingDecls.push_back(FoundDecl);

I am aware of similar errors like this with other AST nodes. We had a patch in our fork to fix that in January (https://github.com/Ericsson/clang/pull/569/files) I am going to prepare a patch from that cause I see now this affects you guys in LLDB too.

This sounds reasonable, let me test it out and make sure it looks good.

[shafik]

@martong your idea does not work b/c default construction DeclarationName() treats it the same as being empty. So if (!Name) is still true.

lib/AST/ASTImporter.cpp

2463

This is correct because either SearchName is Name or it is the name of the typedef for the anonymous enum set via ImportInto(SearchName, D->getTypedefNameForAnonDecl()->getDeclName())

[martong]

In D59665#1442328, @shafik wrote:

@martong your idea does not work b/c default construction DeclarationName() treats it the same as being empty. So if (!Name) is still true.

And did you try moving the push_back to the other scope? I'd expect the the ConflictingDecls to be empty then.

[martong]

lib/AST/ASTImporter.cpp

2463

Okay, this is indeed correct. But then we should make a similar change in VisitRecordDecl too (there we do exactly the same with typedefs).

[martong]

Looks good for me now, but we should make a similar change in VisitRecordDecl too.

[shafik]

In D59665#1442826, @martong wrote:

In D59665#1442328, @shafik wrote:

@martong your idea does not work b/c default construction DeclarationName() treats it the same as being empty. So if (!Name) is still true.

And did you try moving the push_back to the other scope? I'd expect the the ConflictingDecls to be empty then.

Yes, I did and I think it looks good but I have to run a regression.

lib/AST/ASTImporter.cpp

2463

This is fixing a specific bug, so I would want to keep this change specifically related to that and I can add a second review for VisitRecordDecl

[martong]

LGTM!

[a_sidorin]

Hi Shafik,
Thank you for the explanation, it is much more clear to me now. But, as I see, D59692 is going to discard the changes this patch introduces. @martong Gabor, do you expect the changes of this patch to be merged into yours, or should this patch be abandoned?

[martong]

In D59665#1446764, @a_sidorin wrote:

Hi Shafik,
Thank you for the explanation, it is much more clear to me now. But, as I see, D59692 is going to discard the changes this patch introduces. @martong Gabor, do you expect the changes of this patch to be merged into yours, or should this patch be abandoned?

Actually, I'll have to adjust D59692 to not discard these changes (otherwise we might end up a regression in lldb).

[a_sidorin]

Thanks!

lib/AST/ASTImporter.cpp

for (auto *FoundDecl : FoundDecls) {
         if (IsStructuralMatch(D, FoundEnum))
           return Importer.MapImported(D, FoundEnum);
       }
 
       ConflictingDecls.push_back(FoundDecl);
     }
 
     if (!ConflictingDecls.empty()) {
-      Name = Importer.HandleNameConflict(Name, DC, IDNS,
+      Name = Importer.HandleNameConflict(SearchName, DC, IDNS,
                                          ConflictingDecls.data(),
                                          ConflictingDecls.size());
       if (!Name)
         return make_error<ImportError>(ImportError::NameConflict);
     }
   }
 
   SourceLocation ToBeginLoc;