[cfe-dev] [ASTImporter][CrossTU] inconsistent enumerators

[Rafael·Stahl via cfe-dev]

Hello Gábor,

thank you for the fix! I can confirm that this is working for me.

Unfortunately, I don't think I'd be able to properly fix this, because 
it is unclear to me how the code distributes responsibilities. For 
example I wouldn't be able to tell that your fix is a dirty one and 
where else this should be handled. Import(Decl*), VisitEnumDecl, 
VisitEnumConstantDecl, VisitEnumType or a new VisitTagDecl all kind of 
touch this topic and interconnect.

Should I file a bug to make sure this is not forgotten?

Best regards
Rafael


On 16/11/17 17:42, Gábor Horváth wrote:
> Hi Rafael!
>
> Yeah, the cross TU patch is a great way to detect issues with the 
> ASTImporter and unfortunately, those issues are extremely hard to debug.
>
> Here, it looks like the source of the problem is that, when we import 
> an EnumConstantDecl, we will import the EnumDecl without the typdef part.
> A dirty quick fix for this particular case:
>
> @@ -1782,6 +1807,11 @@ Decl 
> *ASTNodeImporter::VisitEnumConstantDecl(EnumConstantDecl *D) {
>    DeclarationName Name;
>    SourceLocation Loc;
>    NamedDecl *ToD;
> +  auto *ED = cast<EnumDecl>(D->getDeclContext());
> +  if (auto *TND = ED->getTypedefNameForAnonDecl()) {
> +    if (!Importer.Import(TND))
> +      return nullptr;
> +  }
>    if (ImportDeclParts(D, DC, LexicalDC, Name, ToD, Loc))
>      return nullptr;
>    if (ToD)
>
> I did not have time yet to deep dive into this and look at what the 
> proper solution would be. Are you willing to continue to investigate this?
>
> Regards,
> Gábor
>
> On 13 November 2017 at 12:16, Rafael·Stahl via cfe-dev 
> <cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>> wrote:
>
>     Deal all
>
>     While using the static analyzer with cross translation unit
>     support ( https://reviews.llvm.org/D30691
>     <https://reviews.llvm.org/D30691> ), I stumbled upon an issue that
>     is most likely related to the ASTImporter.
>
>     I'm using a custom checker, but this should be reproduced by just
>     running the scan-build tool.
>
>     A simple reproducing example:
>
>     main.c:
>
>     ---------------------------------------
>     void foo();
>     void moo();
>
>     int main()
>     {
>         foo();
>         moo();
>     }
>     ---------------------------------------
>
>     foo.c:
>
>     ---------------------------------------
>     #include "thing.h"
>
>     void foo()
>     {
>         (void)THING_VALUE;
>     }
>
>     void conflict(thing_t type)
>     {
>     }
>     ---------------------------------------
>
>     moo.c:
>
>     ---------------------------------------
>     #include "thing.h"
>
>     void moo()
>     {
>         conflict(THING_VALUE);
>     }
>     ---------------------------------------
>
>     thing.h:
>
>     ---------------------------------------
>     typedef enum {
>         THING_VALUE
>     } thing_t;
>
>     void conflict(thing_t type);
>     ---------------------------------------
>
>     Notes on particularities of this example:
>
>     - main.c needs to NOT include the header
>     - main() needs to call the functions in this order
>     - foo() needs to reference the enumerator
>     - the enum needs to be typedef'd
>
>     If any of the above points do not apply, the issue does not appear.
>
>     The issue:
>
>     ---------------------------------------
>     In file included from moo.c:1:
>     thing.h:1:9: warning: type 'thing_t' has incompatible
>           definitions in different translation units [-Wodr]
>     typedef enum {
>             ^
>     thing.h:2:5: note: enumerator 'THING_VALUE' with value 0 here
>         THING_VALUE
>         ^
>     thing.h:1:9: note: no corresponding enumerator here
>     foo.c:8:6: error: external function 'conflict' declared
>           with incompatible types in different translation units
>     ('void (thing_t)' vs. 'void (thing_t)')
>     void conflict(thing_t type)
>          ^
>     thing.h:5:6: note: declared here with type
>           'void (thing_t)'
>     void conflict(thing_t type);
>          ^
>     ---------------------------------------
>
>     After importing conflict(thing_t) the ASTImporter compared the
>     imported one with the original by structural equivalence. This
>     reveals that in the imported enum the enumerators are missing,
>     causing the above error.
>
>     In my real code, not this example, I was unable to dump the
>     imported EnumDecl, because it eventually calls
>     SourceManager::isBeforeInTranslationUnit with two SourceLocations
>     from different files. Not sure if this is related.
>
>     I have tried adding:
>
>     for (const auto Enumerator : D->enumerators())
>         D2->addDecl(Importer.Import(Enumerator));
>
>     before completing the definition in
>     ASTNodeImporter::VisitEnumDecl(), but that results in:
>
>     exe: tools/clang/lib/AST/DeclBase.cpp:1374: void
>     clang::DeclContext::addHiddenDecl(clang::Decl*): Assertion
>     `!D->getNextDeclInContext() && D != LastDecl && "Decl already
>     inserted into a DeclContext"' failed.
>
>     I'm not familiar enough with the ASTImporter to help myself
>     further here. Does anyone know what could be the issue?
>
>     Best regards
>     Rafael Stahl
>
>
>
>     _______________________________________________
>     cfe-dev mailing list
>     cfe-dev at lists.llvm.org <mailto:cfe-dev at lists.llvm.org>
>     http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
>     <http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev>
>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20171117/ccc86c74/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 5449 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20171117/ccc86c74/attachment.bin>