Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add layout rule for [[no_unique_address]] (C++20 P0840R2) #50

Merged
merged 3 commits into from
Jun 20, 2018

Conversation

zygoloid
Copy link
Contributor

Fixes #49

@zygoloid
Copy link
Contributor Author

zygoloid commented Mar 17, 2018

For exposition: the idea here is to use max(nvsize(D),dsize(D)) as the allocated size for potentially-overlapping data members of type D in the same way we use nvsize(D) as the allocated size for base classes. We previously only used dsize(D) as an intermediate value when laying out D, and now becomes part of the data propagated to other classes' layouts.

We need to take the max of these two values because:

  • we want [[no_unique_address]] to give the same layout as a base class whenever possible, which is whenever the field has no virtual bases of its own
  • therefore in the "no virtual bases" case, we want to reserve nvsize(D) bytes (in this case, nvsize(D) is necessarily >= dsize(D))
  • in the remaining cases, we need to reserve dsize(D) to make room for D's virtual bases

Note that there are corner cases where nvsize(C) > dsize(C)! This happens when we (strangely) increase nvsize(C) to include the complete size of an empty base class. (We do need to increase sizeof(C) for this case, but not nvsize(C).) Perhaps that's something we could consider fixing for ABI v2.

The rules could be simplified slightly by setting dsize(C) = max(dsize(C), nvsize(C)) at the end of class layout, and then only using dsize(C) in recursive layouts, but it seems unreasonable to change the dsize for existing classes -- who knows what exciting uses people have found for it :)

@zygoloid zygoloid force-pushed the no-unique-address branch 8 times, most recently from e6d4aa1 to a187d15 Compare April 7, 2018 00:52
@rjmccall
Copy link
Collaborator

@zygoloid, would you mind checking my merge and verifying that this change still reflects the right rule according to the standard?

abi.html Outdated
@@ -164,10 +164,15 @@ <h3><a href="#definitions"> 1.1 Definitions </a></h3>
<dt> <i>empty class</i> </dt>
<dd>
A class with no non-static data members,
no unnamed bit-fields other than zero-width bit-fields,
no unnamed bit-fields other than zero-width bit-fields and empty data members,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the "empty data members" be a qualification on "no non-static data members" rather than "no unnamed bit-fields"?

Copy link
Collaborator

@rjmccall rjmccall May 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, absolutely; that was a mistake introduced by my merge. Fixed.

@zygoloid
Copy link
Contributor Author

Merged rule now looks good, thanks.

@rjmccall
Copy link
Collaborator

Okay. I'll leave this open for a week for further comment.

@rjmccall
Copy link
Collaborator

1 week, 4 weeks, eh.

@rjmccall rjmccall merged commit 8db4972 into itanium-cxx-abi:master Jun 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants