This is an archive of the discontinued LLVM Phabricator instance.

[ASTImporter] Unnamed structs import
ClosedPublic

Authored by szepet on Mar 12 2017, 4:38 PM.

Details

Summary

I think we should check the unnamed structs locations/positions in the context otherwise the Importer thinks in case of the "struct Unnamed" test case that the inner structs types (R and S) should be equivalent and would give a warning because of type mismatch.

On the other hand it is not enough since the findUntaggedStructOrUnionIndex should be able to deal with nested structs when the RecordType is deep inside an ElaboratedType.

With these changes I do not see the point of the removed else statement (L3079).
Test cases added.

Important to notice that:

union { int i; float f; };

is an anonymous (and unnamed) union but

union { int i; float f; } U;

is not anonymous but I referred to it as unnamed. (Hopefully good wording.)

Diff Detail

Repository
rL LLVM

Event Timeline

szepet created this revision.Mar 12 2017, 4:38 PM
a.sidorin edited edge metadata.Mar 20 2017, 3:27 AM

Thank you for catching this! Looks mostly good, I just have some inline questions.

lib/AST/ASTImporter.cpp
1225 ↗(On Diff #91509)

Iterating over ElaboratedTypes rings a bell for me that we may need to use getCanonicalType(). Does this code deals with typedefs well?

test/ASTMerge/struct/Inputs/struct2.c
62 ↗(On Diff #91509)

Please leave a space after // in comments.

112 ↗(On Diff #91509)

Please point that mismatch should occur here, it is very hard to compare structures manually.

szepet updated this revision to Diff 92411.Mar 20 2017, 6:05 PM
szepet marked 2 inline comments as done.

Nits resolved, dependency added in order to not to conflict the 2 patches.

a.sidorin accepted this revision.Mar 20 2017, 6:10 PM

Looks good. I will commit this if there will be no comments from other reviewers. Thank you!

This revision is now accepted and ready to land.Mar 20 2017, 6:10 PM
szepet added inline comments.Mar 20 2017, 6:16 PM
lib/AST/ASTImporter.cpp
1225 ↗(On Diff #91509)

Well, this function does not deal with typedefs but it checked via IsStructuralMatch function call (Line 3055). This is why the importer can recognize that they are different structs. Furthermore this function "returns an empty option if the context is not a record". And typedefs inside structs only allowed in C++ where an unnamed struct typedef would seem strange. So I dont think this function needs to deal with typedefs but if you prefer that way then I can write an ifstmt getting the underlying type in case of a typedef.

Unfortunately this patch can not be applied to current top of tree (due to conflicts in the tests).

This revision was automatically updated to reflect the committed changes.
xazax.hun reopened this revision.Apr 3 2017, 2:20 PM

Reopened until a fix for windows builds.

This revision is now accepted and ready to land.Apr 3 2017, 2:20 PM
dyung added a subscriber: dyung.Apr 3 2017, 6:40 PM

Reopened until a fix for windows builds.

If you aren't going to fix this soon, could you please revert the change so that it doesn't mask other potential problems?

Reopened until a fix for windows builds.

If you aren't going to fix this soon, could you please revert the change so that it doesn't mask other potential problems?

I did revert that before I reopened this.

dkrupp added a reviewer: bruno.Nov 3 2017, 8:49 AM

Ok, https://reviews.llvm.org/D39886 is an independent fix for the same issue. It might be useful to add the test cases from that patch to this one as Aleksei pointed out.
The current status of this one, the windows build bot failed, but we could not reproduce it (even on windows). Is there someone who could reproduce the fail? Should we just recommit this hoping that the fail was unrelated to this patch?

tk1012 added a subscriber: tk1012.Nov 21 2017, 6:55 AM

Hi Gabor and Peter,
I cannot reproduce the issue. I have updated the patch in my local repo and will try to re-commit it tomorrow. Right now, all buildbots are red.

This revision was automatically updated to reflect the committed changes.
dyung added a comment.Apr 5 2018, 4:09 PM

FYI, when this was commited in r329301, it again caused failures on at least the PS4 Windows bot. The problem was due to the explicit "/struct" being searched for in your CHECK lines. I have committed a fix in r329361 which seems to fix it by removing the leading '/' character, so it gets rolled into the regex PATH_TO_INPUTS.

Hi Douglas,

Could you provide the reference to the buildbot? I haven't received any blame e-mails related to the change after the commit.

dyung added a comment.Apr 6 2018, 6:27 PM

Hi Douglas,

Could you provide the reference to the buildbot? I haven't received any blame e-mails related to the change after the commit.

http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/16288

Your change was initially tested in http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/16279, however due to a build failure, your test was not actually run until the build failure was fixed in the above run.

Your change was initially tested in http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-windows10pro-fast/builds/16279, however due to a build failure, your test was not actually run until the build failure was fixed in the above run.

Oh, I see now. Thank you!