Page MenuHomePhabricator

[libclang] Allow field offset lookups in types with incomplete arrays.
ClosedPublic

Authored by JornVernee on Apr 28 2019, 6:19 AM.

Details

Summary

Currently, looking up the offset of a field in a type with an incomplete array, using clang_Type_getOffsetOf always returns CXTypeLayoutError_Incomplete. e.g. for the following:

struct Foo {
    int size;
    void* data[]; // incomplete array
};

This returns CXTypeLayoutError_Incomplete when looking up the offset of either the 'size' or the 'data' field.

But, since an incomplete array always appears at the end of a record, looking up the field offsets should be fine. Note that this also works fine using the offsetof macro.

The fix for this is to ignore incomplete array fields when doing the recursive type validation.

Diff Detail

Event Timeline

JornVernee created this revision.Apr 28 2019, 6:19 AM

Fixed the similar problem with clang_Type_getAlignOf since the patch is pretty small any ways.

@yvvan Mind taking a look here as well?

aaron.ballman added inline comments.May 1 2019, 5:33 AM
test/Index/print-type-size.c
23

This should be the first line of the file. I don't have strong opinions on where the CHECK lines go, but I usually prefer seeing them near the construct being checked.

32

Please add a newline to the end of the file.

tools/libclang/CXType.cpp
902

I don't think IAT is a common enough acronym to use; I'd probably just drop the comment as it adds little value.

964

Same here.

964

I sort of wonder whether we want a helper function like isTypeIncompleteForLayout() or something, and then using that helper directly.

JornVernee updated this revision to Diff 197532.May 1 2019, 5:59 AM
JornVernee marked 2 inline comments as done.
  • Restructured test source file to put CHECK comments inline
  • Removed IAT comments.
  • holding of on adding helper method until hearing back.
JornVernee updated this revision to Diff 197533.May 1 2019, 6:01 AM

Add forgotten newline

  • holding of on adding helper method until hearing back.

My rationale for wanting a helper method is because we already have two places were incompleteness is important but has special cases. It's easier to maintain that with something that's a named function to explain what the predicate is actually doing. My concern is that we add another isIncompleteType() and not think about this issue.

Do we need this in validateFieldParentType()?

JornVernee marked 4 inline comments as done.EditedMay 1 2019, 7:03 AM
  • holding of on adding helper method until hearing back.

My rationale for wanting a helper method is because we already have two places were incompleteness is important but has special cases. It's easier to maintain that with something that's a named function to explain what the predicate is actually doing. My concern is that we add another isIncompleteType() and not think about this issue.

Do we need this in validateFieldParentType()?

Thanks for the response, I misunderstood.

I usually go with naming my predicates in that way as well, but I misunderstood where you wanted to use it. I think adding the helper method for validateFieldParentType() is good. But, the check in clang_Cursor_getAlignOf is semantically different, since we basically want to check if the type has an alignment. (I actually realized we also need to check the element type for completeness in the case of incomplete arrays. Figuring that out now).

test/Index/print-type-size.c
23

This style was taken from print-type-size.cpp, which also puts everything at the end. I'll put it at the front/inline instead (makes sense to me as well). I guess it's a legacy thing we're trying to move away from?

tools/libclang/CXType.cpp
964

Note that clang_Type_getSizeOf will (and AFAIK should) still return CXTypeLayoutError_Incomplete for incomplete arrays. I'd expect a potential isTypeIncompleteForLayout() to be used by all three functions (OffsetOf, SizeOf, AlignOf). So, I'm in favour of doing without such a helper method for the special OffsetOf and AlignOf cases.

It's a pretty unique case (but it appears a bunch of times in Windows API headers so we'd like to have support for it :) ).

JornVernee updated this revision to Diff 197543.May 1 2019, 7:24 AM
JornVernee marked an inline comment as done.
  • Replaced plane predicates with helper methods describing their function.
  • Fixed test file indentation

Changing the logic for clang_Type_getAlignOf was not needed after all, since the use of incomplete type as array element type was already causing an error earlier on (so I think we can assume at that point it's safe?)

aaron.ballman added inline comments.May 1 2019, 7:51 AM
test/Index/print-type-size.c
7

Can you run this through clang-format so it matches our usual formatting style rules?

tools/libclang/CXType.cpp
888

No need to use a reference here, just pass QualType (non-const).

957

Similar here.

JornVernee updated this revision to Diff 197557.EditedMay 1 2019, 8:25 AM
JornVernee marked an inline comment as done.
  • Removed const& QualType and passing by-value instead
  • Ran print-type-size.c through clang-format
JornVernee marked 3 inline comments as done.

Removed spurious newline

aaron.ballman accepted this revision.May 1 2019, 12:50 PM

LGTM, thank you! That's strange that clang-format would remove the newline from the end of the file. I don't get that behavior when I try it, so it makes me wonder what's different.

This revision is now accepted and ready to land.May 1 2019, 12:50 PM

LGTM, thank you! That's strange that clang-format would remove the newline from the end of the file. I don't get that behavior when I try it, so it makes me wonder what's different.

Yeah, my mistake. It was there after all. The output from clang-format looked the same as what I had before, but I think I just didn't catch the newline when copy-pasting the diff the first time :)

Also, I don't have committer access. Would you mind committing this?

LGTM, thank you! That's strange that clang-format would remove the newline from the end of the file. I don't get that behavior when I try it, so it makes me wonder what's different.

Yeah, my mistake. It was there after all. The output from clang-format looked the same as what I had before, but I think I just didn't catch the newline when copy-pasting the diff the first time :)

Ah that makes more sense, phew!

Also, I don't have committer access. Would you mind committing this?

Happy to do so -- I'll try to get it in this week if I have the time, but it may not happen until early next week.

Also, I don't have committer access. Would you mind committing this?

Happy to do so -- I'll try to get it in this week if I have the time, but it may not happen until early next week.

Take your time. Thanks for al the help!

aaron.ballman closed this revision.May 7 2019, 6:58 AM

I've commit in r360147, thank you for the patch!