Page MenuHomePhabricator

[Sema] PR25755 Handle out of order designated initializers
Needs ReviewPublic

Authored by hintonda on Feb 18 2016, 12:58 PM.

Details

Reviewers
rsmith
CornedBee
Summary

Keep track of which fields have been initialized when designated initializers are used, and only verify the unseen ones can be implicitly initialized them at the top level object.

Event Timeline

hintonda updated this revision to Diff 48389.Feb 18 2016, 12:58 PM
hintonda retitled this revision from to [Sema] PR25755 Fix crash when initializing out-of-order struct references.
hintonda updated this object.
hintonda added a reviewer: rsmith.
hintonda added a subscriber: cfe-commits.
hintonda updated this revision to Diff 48561.Feb 19 2016, 5:06 PM
  • Fix additional tests
hintonda updated this revision to Diff 48815.Feb 23 2016, 6:48 AM

Moved new test into existing test file.

Current diff is incorrect, but I did find the problem.

When iterating over the fields of a record, we use an iterator. The problem is that if we use a designator, we reset the iterator value, so we can no longer check Field != FieldEnd if the designators are out of order.

So, if we want to allow designators to be out of order, we'll need to keep track of what was actually initialized, not just that we initialized the correct number. (see SemaInit.cpp:1825)

I'll work up a fix.

hintonda updated this revision to Diff 49281.Feb 26 2016, 9:01 PM

When checking record initialization, maintain a vector of remaining
fields that were not explicitly initialized and check only those
fields to see if they can be default initialized.

hintonda updated this object.Feb 26 2016, 9:07 PM
hintonda updated this revision to Diff 49295.Feb 27 2016, 8:21 AM

Improve efficiency by using pair<> to maintain counts instead of purging seen fields.

hintonda updated this revision to Diff 49772.Mar 3 2016, 1:47 PM

Improve efficiency by only keeping track of seen fields, and only when
designators have been used.

rsmith added inline comments.Mar 18 2016, 10:49 AM
lib/Sema/SemaInit.cpp
1732 ↗(On Diff #49772)

Please use an llvm::BitVector here instead.

1756–1769 ↗(On Diff #49772)

You can get the field index with Field->getFieldIndex() - 1 rather than computing it here (though watch out for the case where Field == FieldEnd).

1793–1795 ↗(On Diff #49772)

We don't need to track whether unnamed bit-fields have been "seen". This isn't meaningful.

1808–1809 ↗(On Diff #49772)

Once we're in the hadError state, we don't need to track SeenFields any more. I don't think we need this change.

hintonda updated this revision to Diff 51099.Mar 18 2016, 7:28 PM

Addressed comments.

rsmith added inline comments.Mar 19 2016, 4:07 AM
lib/Sema/SemaInit.cpp
1815–1816 ↗(On Diff #51099)

You don't need either of these variables.

1841–1850 ↗(On Diff #51099)

Drop this. Instead, check for CheckForMissingFields in the loop below.

1852 ↗(On Diff #51099)

Use RD->getNumFields() - 1 here.

1914 ↗(On Diff #51099)

Use Field->getFieldIndex()

1936–1954 ↗(On Diff #51099)

Don't build a list of missing fields, just walk all the fields and check whether each one is in the set before processing it. (You can optimize this somewhat: if there were any designators -- including the case where Field was not RD->field_begin() on entry to this function -- start from RD->field_begin(), otherwise start from Field.)

hintonda updated this revision to Diff 51114.Mar 19 2016, 9:32 AM

Address additional comments.

Always maintain SeenFields, not just when designators are seen which
simplies the logic. Note that there is no getNumFields() method, so
we still need to determine the total FieldSize to initialize the
BitVector and loop through it.

hintonda marked 7 inline comments as done.Mar 28 2016, 5:30 PM
hintonda added inline comments.
lib/Sema/SemaInit.cpp
1815–1816 ↗(On Diff #51114)

Still need FieldSize -- see below.

1856 ↗(On Diff #51114)

Couldn't find getNumFields(), counted them instead.

rsmith added inline comments.Mar 28 2016, 5:59 PM
lib/Sema/SemaInit.cpp
1818–1823 ↗(On Diff #51114)

FieldStart isn't necessarily the first field (in particular, if we got here from a nested field designator). Instead, how about using

FieldSize = std::distance(RD->field_begin(), RD->field_end());
1922 ↗(On Diff #51114)

The CheckForMissingFields test here doesn't make much sense with this name. Please reverse the sense of this flag, rename it to HadAnyDesignators or similar, and initialize it to true if Field != RD->field_begin() before you enter the field initialization loop.

1923 ↗(On Diff #51114)

Start from RD->field_begin() here (or track the position at which you first set HadAnyDesignators to true and start from there). In particular, if FieldStart != RD->field_begin(), then an initial sequence of fields is uninitialized unless a later designator jumped back to them.

1926–1927 ↗(On Diff #51114)

Use SeenFields[Field->getFieldIndex()] here and then remove the variable i. We don't need to hardcode assumptions about how field indexes are computed here.

I'm probably missing something, but isn't InitListChecker::CheckStructUnionTypes() called recursively, if indirectly? In which case we'd want to start from the Field iterator we were given, not the look at all the fields.

hintonda added inline comments.Mar 28 2016, 7:07 PM
lib/Sema/SemaInit.cpp
1926–1927 ↗(On Diff #51114)

You raise a good point I missed. SeenFields should be big enough to mark all fields, but we should only set/examine from FieldStart->getFieldIndex(). However, we still need the i.

rsmith edited edge metadata.Apr 7 2016, 6:23 PM

I'm probably missing something, but isn't InitListChecker::CheckStructUnionTypes() called recursively, if indirectly? In which case we'd want to start from the Field iterator we were given, not the look at all the fields.

No: if CheckDesignatedInitializer finds a top-level designator (denoting a field in the same struct for which it was called, rather than a subobject of that field), it just returns. CheckStructUnionTypes should never recursively re-enter the same subobject.

However, you do make a good point: we *can* enter CheckStructUnionTypes multiple times for the same subobject, in a case like

struct Q { Q(int); }
struct A { Q x, y; };
struct B { A a; int k; };
B b = { .a.y = 0, 0, .a.x = 0 };

... and we should not CheckEmptyInitializable in that case, nor warn that an initializer for a.x is missing. (But we should do so if the final initializer for a.x is not present.)

It looks like we need to do all this checking as a separate pass after checking the initialization, even in VerifyOnly mode.

hintonda updated this revision to Diff 191828.Mar 22 2019, 12:28 AM
  • Initial checkin from original D17407.
  • Refactored code and simplify tests based on review comments.
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2019, 12:28 AM
Herald added a subscriber: jdoerfert. · View Herald Transcript
hintonda retitled this revision from [Sema] PR25755 Fix crash when initializing out-of-order struct references to [Sema] PR25755 Handle out of order designated initializers .Mar 22 2019, 12:36 AM
hintonda edited the summary of this revision. (Show Details)

Thanks for pointing that out. I was mainly handling asserts, but will add the restrictions as well.

I took a look at C++ designator restrictions shown here aggregate initialization, and believe it to be orthogonal to this change. Thus, I'd prefer to put that in a separate patch.

However, based on this, I also believe I should move the tests into test/Sema/designated-initializers.c.

Please let me know if that sounds like the right thing to do.

If you compile with -pedantic, you'll get the following warning: warning: designated initializers are a C99 feature [-Wc99-extensions]

This warning currently applies for all versions of C++, but should probably not warn for cases supported in c++20. However, that's a bigger fix, since it would need to discriminate between what c++20 does and does not support, e.g., nested and out of order designators are not included in c++20.

@rsmith This is ready for review when you get a chance.