Page MenuHomePhabricator

[clang][AST] Check context of record in structural equivalence.

Authored by balazske on Nov 3 2021, 8:51 AM.



The AST structural equivalence check did not differentiate between
a struct and a struct with same name in different namespace. When
type of a member is checked it is possible to encounter such a case
and wrongly decide that the types are similar. This problem is fixed
by check for the namespaces of a record declaration.

Diff Detail

Event Timeline

balazske created this revision.Nov 3 2021, 8:51 AM
balazske requested review of this revision.Nov 3 2021, 8:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 3 2021, 8:51 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I very much like the code.
L1364 is uncovered according to my coverage results.


Shouldn't you also check the name as well?


So, the previous DC1->isTranslationUnit() guards the DC1->getParent() to never be NULL; same for DC2.
Pretty neat.


The outer-most namespace could also be inline.
I'd love to see a test about that as well.


extern "C" might be nested within other namespaces.
Probably worth testing that as well.


Please, also test if the name of the structs is mismatched.

How does it handle this case:

struct A {int x;};

struct A f(void) {
    struct A{} a;

    return a;

and this case:

struct b{};

struct a {int x;}           // visible outside of f
f(struct b {int x; } b1) {  // not visibile out of f
  return (struct a){.x = 1};

In the last case the the RecordDecl's a and b can be matched (not internal statements in a function) and this is similar to the existing test cases. The struct a and struct b already fails to match because different name.


Here DC1 or DC2 is already a function or translation unit, the name of it is not relevant.


Change to !DC1->getParent() (from DC1->isTranslationUnit())? (The line can be moved to here to increase readability, so the break statement is at end of the loop.)

balazske updated this revision to Diff 387181.Nov 15 2021, 2:28 AM

Updated tests.

martong accepted this revision.Nov 19 2021, 2:22 AM

Thanks @balazske for updating the tests, I think this addresses @shafik 's questions. LGTM! Ping @shafik

This revision is now accepted and ready to land.Nov 19 2021, 2:22 AM
balazske updated this revision to Diff 389111.Nov 23 2021, 1:04 AM

Applied lint reformattings.

martong accepted this revision.Nov 23 2021, 8:08 AM

Still LGTM! Let's land it!

This revision was landed with ongoing or failed builds.Nov 24 2021, 8:20 AM
This revision was automatically updated to reflect the committed changes.

Looks like this breaks tests on windows:

Please take a look and revert for now if it takes a while to fix.

balazske reopened this revision.Nov 25 2021, 2:45 AM

Windows test failures occurred because wrong AST matchers in the tests. Tests will be updated to use named match.

This revision is now accepted and ready to land.Nov 25 2021, 2:45 AM
balazske updated this revision to Diff 389709.Nov 25 2021, 2:55 AM

Using correct AST match in the tests to fix failures on Windows.

shafik added inline comments.Nov 29 2021, 1:58 PM

I expect this to be false the outer Param is not the same type as the Param in the parameter.


I expect this to be false as well.

balazske added inline comments.Nov 30 2021, 1:16 AM

There is a test ctu-main.c that fails if this is false. In that code a similar construct as here (structInProto) is imported and the import fails because unsupported construct. If these are structurally not equivalent then the failure is a ODR like error (the Param inside the function prototype is different than the Param outside of it). I do not know which is the correct behavior, I think the intent in that test is that import of this construct should work (if it would be supported by ASTImporter) at least in the C case.

shafik added inline comments.Nov 30 2021, 12:14 PM

They are compatible types, so if that is the criteria then that looks reasonable.