[ASTImporter][CrossTU] inconsistent enumerators

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

[ASTImporter][CrossTU] inconsistent enumerators

David Chisnall via cfe-dev
Deal all

While using the static analyzer with cross translation unit support (
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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

smime.p7s (7K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [ASTImporter][CrossTU] inconsistent enumerators

David Chisnall via cfe-dev
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 <[hidden email]> wrote:
Deal all

While using the static analyzer with cross translation unit support ( 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: [ASTImporter][CrossTU] inconsistent enumerators

David Chisnall 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 <[hidden email]> wrote:
Deal all

While using the static analyzer with cross translation unit support ( 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
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev




_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-dev

smime.p7s (7K) Download Attachment