This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Add support for importing GenericSelectionExpr AST nodes.
ClosedPublic

Authored by tmroeder on Dec 3 2020, 12:39 PM.

Details

Summary

This allows ASTs to be merged when they contain GenericSelectionExpr
nodes (this is _Generic from C11). This is needed, for example, for
CTU analysis of C code that makes use of _Generic, like the Linux
kernel.

The node is already supported in the AST, but it didn't have a matcher
in ASTMatchers. So, this change adds the matcher and adds support to
ASTImporter. Additionally, this change adds support for structural
equivalence of _Generic in the AST.

Diff Detail

Event Timeline

tmroeder created this revision.Dec 3 2020, 12:39 PM
tmroeder requested review of this revision.Dec 3 2020, 12:39 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 3 2020, 12:39 PM
tmroeder updated this revision to Diff 309344.Dec 3 2020, 12:58 PM
tmroeder edited the summary of this revision. (Show Details)

Fix a typo in the summary.

shafik added a comment.Dec 3 2020, 1:52 PM

ASTImporter changes looks good to me.

clang/lib/AST/ASTImporter.cpp
6525

nitpick context -> ToCtx to be consistent with other code.

tmroeder updated this revision to Diff 309406.Dec 3 2020, 4:54 PM

Renamed context to ToCtx as suggested.

It might be worth to extend StmtComparer as well with this Expr. I mean, probably two selections exprs should be considered equal if their assoc exprs are equal too...

It might be worth to extend StmtComparer as well with this Expr. I mean, probably two selections exprs should be considered equal if their assoc exprs are equal too...

@teemperor , WDYT?

Having the comparison code would be nice. The custom types in the associated types aren't compared by the generic code, so an overload like this in the StmtComparer class should be enough:

bool IsStmtEquivalent(const GenericSelectionExpr *E1, const GenericSelectionExpr *E2) {
  for (auto Pair : zip_longest(E1->getAssocTypeSourceInfos(),
                               E2->getAssocTypeSourceInfos())) {
    Optional<TypeSourceInfo *> Child1 = std::get<0>(Pair);
    Optional<TypeSourceInfo *> Child2 = std::get<1>(Pair);
    // Different number of associated types.
    if (!Child1 || !Child2)
      return false;

    if (!IsStructurallyEquivalent(Context, (*Child1)->getType(),
                                  (*Child2)->getType()))
      return false;
  }
  return true;
}
aaron.ballman added inline comments.Dec 7 2020, 11:33 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
2366

Do we have a use case for adding this as an AST matcher? For instance, one of the first things I'd want to do with such a matcher is traverse down to the association list, but I don't think that's possible currently. Should we add all of the AST functionality or just wait until there's a concrete use case and add the AST matchers bits at that point?

clang/lib/AST/ASTImporter.cpp
6509
tmroeder added inline comments.Dec 7 2020, 3:48 PM
clang/include/clang/ASTMatchers/ASTMatchers.h
2366

I mostly did this because that's what I did in the previous case from Feb 2019: reviews.llvm.org/D58292, for ChooseExpr, and I was cribbing off my old code.

However, on further reflection, I remembered that this patch actually removes a case that was locally defining this matcher already: see the removed lines in clang/lib/Analysis/ExprMutationAnalyzer.cpp in this patch. So, I think that is enough of a case for adding this.

tmroeder updated this revision to Diff 310047.Dec 7 2020, 4:05 PM

Added structural equivalence and a few tests for it.

tmroeder updated this revision to Diff 310050.Dec 7 2020, 4:20 PM

Fixed the clang-tidy warning as suggested.

tmroeder marked an inline comment as done.Dec 7 2020, 4:21 PM
aaron.ballman added inline comments.Dec 8 2020, 6:11 AM
clang/include/clang/ASTMatchers/ASTMatchers.h
2366

Okay, fair enough. Thanks!

clang/test/ASTMerge/generic-selection-expr/Inputs/generic.c
2

Should we also have a C++ test for a result-dependent use of _Generic? (We support use of _Generic in C++ as an extension.)

clang/unittests/AST/StructuralEquivalenceTest.cpp
1601 ↗(On Diff #310050)

Should we add a structural equivalence test for a dependent result type in C++?

tmroeder updated this revision to Diff 310394.Dec 8 2020, 5:40 PM
tmroeder edited the summary of this revision. (Show Details)

Added C++ tests for result-dependent uses, and added that fact to the description as well.

tmroeder marked 2 inline comments as done.Dec 8 2020, 5:41 PM
tmroeder added inline comments.
clang/test/ASTMerge/generic-selection-expr/Inputs/generic.c
2

Added in a new file test.cpp

tmroeder marked an inline comment as done.Dec 8 2020, 5:44 PM
This revision is now accepted and ready to land.Dec 9 2020, 12:24 PM

@aaron.ballman It looks like those template tests fail on x64 windows; this is a problem that came up with D58292 as well. It ended up getting rolled back because template-based tests failed on windows, and I had to remove the tests and resubmit.

I'm not sure what the right way is to fix these tests: skip them on Windows? If so, then can you please tell me how to do that?

tmroeder updated this revision to Diff 310701.Dec 9 2020, 3:59 PM

Moved the result-dependent tests into a different test class, one that explicitly uses templates already.

martong accepted this revision.Dec 10 2020, 2:54 AM

LGTM!

@aaron.ballman It looks like those template tests fail on x64 windows; this is a problem that came up with D58292 as well. It ended up getting rolled back because template-based tests failed on windows, and I had to remove the tests and resubmit.

I'm not sure what the right way is to fix these tests: skip them on Windows? If so, then can you please tell me how to do that?

Looking at the changes you made, I'm not certain why one test would work and one test would fail -- however, the usual issue with template tests on Windows is that the default behavior on Windows is to match the MSVC template behavior and so you sometimes need to pass -fno-delayed-template-parsing to those tests to get the same behavior as on other platforms.

I wouldn't worry much about the ASTMerge tests, those are mostly legacy and not actively used nowadays. (At one point we were thinking even to remove them because we could got rid of the ASTMergeAction which is exclusively used for testing.)
The main test infrastructure for the ASTImporter is in the unittests. Those tests are exercised even with -fdelayed-template-parsing and with -fms-compatibility. So I am fine with marking the ASTMerge tests with XFAIL on windows.

tmroeder updated this revision to Diff 310930.Dec 10 2020, 9:12 AM

Fixed a formatting problem brought up by clang-format.

I wouldn't worry much about the ASTMerge tests, those are mostly legacy and not actively used nowadays. (At one point we were thinking even to remove them because we could got rid of the ASTMergeAction which is exclusively used for testing.)
The main test infrastructure for the ASTImporter is in the unittests. Those tests are exercised even with -fdelayed-template-parsing and with -fms-compatibility. So I am fine with marking the ASTMerge tests with XFAIL on windows.

Sorry for the confusion: I uploaded a change to try to fix the real test failure and there turned out to be a clang-format problem in my ASTMerge tests as well. The problems I was referring to were not in ASTMerge but rather from the unit tests in a previous diff: https://buildkite.com/llvm-project/premerge-checks/builds/19205#4f1d9b0a-16bf-401d-b07e-cceb29612fd0

Is there a way to mark unit tests as failing on a particular architecture?

The answer might not matter, because I've moved those unit tests into a different unit-test class, one that only has templates, and I hope that that means it will work correctly or be skipped correct in cases where template processing is a problem.

I haven't so far been able to get my Windows box to build LLVM (still working through some problems like atlbase.h), so I'll have to wait for the CI to tell me if x64 windows works with the diff I just uploaded: it fixes the trivial clang-format problem in the ASTMerge tests and it keeps the Structural Equivalence unit tests in the unit-test class I just mentioned.

tmroeder updated this revision to Diff 310974.Dec 10 2020, 11:59 AM

Rebased to the latest head version.

tmroeder updated this revision to Diff 311079.Dec 10 2020, 5:47 PM

I think I've fixed the Windows tests; I got a build working on a Windows machine and was able to debug.

It looks like the problem is again due to delayed template parsing: the templatized functions in the test both came out as nullptr. So, I added a check for nullptr in the case that expects the two values to be different. The test only tries to compare them if at least one is not null.

tmroeder updated this revision to Diff 311081.Dec 10 2020, 5:51 PM

Removed an unintentionally added extra line.

martong accepted this revision.Dec 14 2020, 8:11 AM

Thanks for the update. I checked it, still looks good to me.

Some notes in parenthesis:

It looks like the problem is again due to delayed template parsing: the templatized functions in the test both came out as nullptr. So, I added a check for nullptr in the case that expects the two values to be different. The test only tries to compare them if at least one is not null.

Ideally, we should have parameterized structural equivalency tests (similarly to what we have in ASTImporterTest with INSTANTIATE_TEST_CASE_P). The parameters could be the compiler options, i.e. in this case I think we would be able to explicitly emit the delayed template parsing option. Anyway, let's keep that as a future update if we bump into more similar cases.

Thanks for the update. I checked it, still looks good to me.

Some notes in parenthesis:

It looks like the problem is again due to delayed template parsing: the templatized functions in the test both came out as nullptr. So, I added a check for nullptr in the case that expects the two values to be different. The test only tries to compare them if at least one is not null.

Ideally, we should have parameterized structural equivalency tests (similarly to what we have in ASTImporterTest with INSTANTIATE_TEST_CASE_P). The parameters could be the compiler options, i.e. in this case I think we would be able to explicitly emit the delayed template parsing option. Anyway, let's keep that as a future update if we bump into more similar cases.

Thanks! Unfortunately, my fix didn't solve the whole problem on Windows, because I forgot to build with assertions when repro'ing the failure, so I fixed one problem but not the other. I'm now working on fixing the test under assertions as well.

tmroeder updated this revision to Diff 311706.Dec 14 2020, 2:13 PM

Change the DeclMatcher.h test code to expose the ability to not assert on null Nodes from a match.

This allows the tests for GenericSelectionExpr to handle the null case on Windows themselves without crashing, and to still work correctly and test the appropriate functionality on Linux.

Change the DeclMatcher.h test code to expose the ability to not assert on null Nodes from a match.

This allows the tests for GenericSelectionExpr to handle the null case on Windows themselves without crashing, and to still work correctly and test the appropriate functionality on Linux.

It looks like this change finally fixed the Windows tests that I was breaking. However, I'm now touching new parts of the test framework to make this work. Please take a look at and let me know what you think.

tmroeder updated this revision to Diff 311738.Dec 14 2020, 4:11 PM

Update to the latest head. No changes from previous diff.

martong requested changes to this revision.Dec 15 2020, 3:14 AM
martong added inline comments.
clang/unittests/AST/StructuralEquivalenceTest.cpp
1656–1662 ↗(On Diff #311738)

Change the DeclMatcher.h test code to expose the ability to not assert on null Nodes from a match.

The original idea was to assert in the DeclMatcher to indicate a failure in the test itself. In the past it happened to me often that I wrote a wrong matcher and that assertion indicated it and that was really helpful.

So, I'd suggest an alternative approach instead of this change in the DeclMatcher: Try to instantiate the template function itself, so even if the delayed template instantiation option is set, the compiler must instantiate.

Related code in other ASTImporter tests:
https://github.com/llvm/llvm-project/blob/bd0709266911bce2f1e8a875f9ed49d56953f323/clang/unittests/AST/ASTImporterGenericRedeclTest.cpp#L60

This revision now requires changes to proceed.Dec 15 2020, 3:14 AM
tmroeder updated this revision to Diff 311961.Dec 15 2020, 10:29 AM

Explicitly instantiate the templates in the structural-equivalence test.

This seems to work correctly on Windows, and it avoids the need for the extra hooks in the test framework. So, this diff removes those hooks as well as the nullptr test. The latter is no longer needed either because the templates get explicitly instantiated, even under delayed template expansion.

martong accepted this revision.Dec 15 2020, 2:28 PM

Thanks for your work, I am glad that finally the Window bug is fixed with a simple solution.

This revision is now accepted and ready to land.Dec 15 2020, 2:28 PM
tmroeder updated this revision to Diff 312256.Dec 16 2020, 10:38 AM

Rebase to the latest head, mostly to force the CI infrastructure to run again now that it is said to work again: https://github.com/google/llvm-premerge-checks/issues/268#issuecomment-745979134.

Looks like I missed the transition of my credentials from SVN from github, so I can't currently commit this.

How do I go about getting those credentials restored?

Looks like I missed the transition of my credentials from SVN from github, so I can't currently commit this.

How do I go about getting those credentials restored?

Never mind, I found it in the Developer Policy document

This revision was landed with ongoing or failed builds.Dec 16 2020, 3:41 PM
This revision was automatically updated to reflect the committed changes.