[ASTImporter][CrossTU] inconsistent enumerators

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

[ASTImporter][CrossTU] inconsistent enumerators

Robinson, Paul 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

Robinson, Paul 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

Robinson, Paul 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
Reply | Threaded
Open this post in threaded view
|

Re: [ASTImporter][CrossTU] inconsistent enumerators

Robinson, Paul via cfe-dev
+ Aleksei

Hi Aleksei!

Did you encounter this problem? Do you have a solution?
What I do not like about this one, the Importer will end up visiting some of the nodes twice.

Regards,
Gábor

On 17 November 2017 at 15:33, Rafael·Stahl <[hidden email]> wrote:

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
Reply | Threaded
Open this post in threaded view
|

Re: [ASTImporter][CrossTU] inconsistent enumerators

Robinson, Paul via cfe-dev
Hello Gabor,

I did not encounter the problem before. Would you let me to investigate the problem a bit more?


30.11.2017 15:17, Gábor Horváth пишет:
+ Aleksei

Hi Aleksei!

Did you encounter this problem? Do you have a solution?
What I do not like about this one, the Importer will end up visiting some of the nodes twice.

Regards,
Gábor

On 17 November 2017 at 15:33, Rafael·Stahl <[hidden email]> wrote:

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





-- 
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics

_______________________________________________
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

Robinson, Paul via cfe-dev
In reply to this post by Robinson, Paul via cfe-dev
Hello guys,

Do you have a reprocase I can just launch clang against? I have some ideas but ASTMerge and clang-import-test infrastructures are not very suitable for testing CSA XTU import strategy.


16.11.2017 19:42, Gábor Horváth 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


-- 
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics

_______________________________________________
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

Robinson, Paul via cfe-dev
In reply to this post by Robinson, Paul via cfe-dev
Hello Gabor,

This code will work, but it seems to be only a partial fix.
You pointed the source of failure correctly. The whole problem is bigger: the code responsible for importing definitions needs some refactoring as well. I will prepare the patch after getting a test case - it is not trivial now.


16.11.2017 19:42, Gábor Horváth 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


-- 
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics

_______________________________________________
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

Robinson, Paul via cfe-dev
Hi Aleksei,

Sorry for the late response.

On 4 December 2017 at 16:56, Aleksei Sidorin <[hidden email]> wrote:
Hello Gabor,

This code will work, but it seems to be only a partial fix.
You pointed the source of failure correctly. The whole problem is bigger: the code responsible for importing definitions needs some refactoring as well. I will prepare the patch after getting a test case - it is not trivial now.

Yeah, unfortunately, this is really hard to reproduce without CTU. Looking forward to your patch. I agree that we do need some refactoring, this eager what we are doing right now is causing lots of troubles. This is not how the parser builds the AST, and I think the closer we can get in this regard, the fewer bugs will we have.
 


16.11.2017 19:42, Gábor Horváth 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


-- 
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics


_______________________________________________
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

Robinson, Paul via cfe-dev
In reply to this post by Robinson, Paul via cfe-dev

Hello Aleksei,

Thank you for looking into this.

Find attached a project of the files I mentioned in my initial report. If you check out something like r318000 and apply the current CTU patch ( https://reviews.llvm.org/D30691 Diff11 is current as of writing), the issue should be visible.

@Gábor Since I run the analyzer from a custom library: Can you tell Aleksei how to run scan-build on the project? As far as I understand it should be possible with the current patch, right?

Regards
Rafael

On 03/12/17 19:22, Aleksei Sidorin wrote:
Hello guys,

Do you have a reprocase I can just launch clang against? I have some ideas but ASTMerge and clang-import-test infrastructures are not very suitable for testing CSA XTU import strategy.


16.11.2017 19:42, Gábor Horváth 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


-- 
Best regards,
Aleksei Sidorin,
SRR, Samsung Electronics


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

enumbug_repro.tar.bz2 (1K) Download Attachment
smime.p7s (7K) Download Attachment