[ASTImporter][CrossTU] inconsistent enumerators

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:

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(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

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

  • 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

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 пишет:

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 пишет:

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 Aleksei,

Sorry for the late response.

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 ( 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

enumbug_repro.tar.bz2 (846 Bytes)

Hello,

It seems like the patch provided by Gábor is not sufficient anymore to fix the issue I was having.

I compiled 6.0_rc1 with the CTU patch and the dirty fix and am getting the original error again.

Best regards
Rafael

Hi Rafael,

I'm close to finish the testing infrastructure for complex import cases (ASTImporterTest-based). I hope to create a review containing both new infra and the fix (as a test for the tests) in two weeks.

12.02.2018 16:35, Rafael·Stahl пишет:

Hello Aleksei,

Since everything has landed on trunk now, even with command line interface for CTU in clang, you can now “just launch clang” :slight_smile:

I have attached the configured project again with a run script containing the command reproducing the error.

Hope this helps!

Regards
Rafael

enumbug_repro.tar.gz (1.08 KB)

Hi Rafael,

This should already be fixed in upstream: ⚙ D44079 [ASTImporter] Allow testing of import sequences; fix import of typedefs for anonymous decls, https://reviews.llvm.org/rL330704. Or is it still possible to reproduce the issue?

03.05.2018 16:00, Rafael·Stahl пишет:

Sorry I didn’t notice that patch because of badly set up mail filters.

I have tested my repro with trunk and the issue is resolved.

Thanks!