Page MenuHomePhabricator

[AST] Move back BasePathSize to the bit-fields of CastExpr
ClosedPublic

Authored by riccibruno on Jan 5 2019, 1:03 PM.

Details

Summary

Background:

The number of trailing CXXBaseSpecifiers in CastExpr was moved from
CastExprBitfields to a trailing object in r338489 (D50050). At this time these bit-fields
classes were only 32 bits wide. However later r345459 widened these bit-field classes to 64 bits.

The reason for this change was that on 64 bit archs alignment requirements caused 4 bytes of
padding after the Stmt sub-object in nearly all expression classes. Reusing this padding yielded
an >10% reduction in the size used by all statement/expressions when parsing all of Boost (on a
64 bit arch). This increased the size of statement/expressions for 32 bits archs, but this can be
mitigated by moving more data to the bit-fields of Stmt (and moreover most people now care
about 64 bits archs as a host).

Therefore move back the number of CXXBaseSpecifiers in CastExpr to the bit-fields of Stmt.
This in effect mostly revert r338489 while keeping the added test.

Diff Detail

Repository
rC Clang

Event Timeline

riccibruno created this revision.Jan 5 2019, 1:03 PM
riccibruno marked 2 inline comments as done.Jan 5 2019, 1:07 PM
riccibruno added inline comments.
include/clang/AST/Expr.h
3030

It cannot overflow now, but someone in the future is going
to shrink CastExprBitfields::BasePathSize.

include/clang/AST/Stmt.h
487

Feel free to find a better comment here.

lebedev.ri added inline comments.Jan 5 2019, 1:14 PM
include/clang/AST/Expr.h
3030

Can you move that static assert here instead of deleting it?

riccibruno marked an inline comment as done.Jan 5 2019, 1:31 PM
riccibruno added inline comments.
include/clang/AST/Expr.h
3030

I am not sure it is really useful since when someone will want to
shrink it, it will be stored as unsigned BasePathSite : x;. What we
don't want is having x too small. However numeric_limits will
be based on the underlying type of the bit-field and so will never fail.

Storing it as something other than an unsigned, say a short, would work,
but it will pack poorly with MSVC.

Hopefully the comment in CastExprBitfields will tell people to not shrink it
mindlessly.

rjmccall added inline comments.Jan 7 2019, 12:03 AM
include/clang/AST/Expr.h
3030

I agree that the type-based numeric limit is not useful if we're anticipating
it becoming a bit-field. The comment seems fine since it's in the code that
would need to be edited.

lebedev.ri accepted this revision.Jan 7 2019, 1:25 AM

LG in principle.

include/clang/AST/Expr.h
3030

I'm just worried that now there will be less of a test coverage,
The actual run-time test already does not test template instantiation with depth of 16384, but much less.

This revision is now accepted and ready to land.Jan 7 2019, 1:25 AM
riccibruno marked an inline comment as done.Jan 7 2019, 5:08 AM
riccibruno added inline comments.
include/clang/AST/Expr.h
3030

Would it make sense to go systematically through the limits recommended by
[implimits], and for each limit devise a test which will exercise it ?

Or would this take too long to be a test ?

It would perhaps also be nice to document these limits somewhere,
even if it is just "You can rely on [implimits]".

lebedev.ri added inline comments.Jan 7 2019, 5:12 AM
include/clang/AST/Expr.h
3030

Would it make sense to go systematically through the limits recommended by
[implimits], and for each limit devise a test which will exercise it ?

Better test coverage is always good.

Or would this take too long to be a test ?

The problem is, even this you can't really test with a run-time test.
Template instantiation is recursive, so in most environments you overflow
your stack long before you reach the recommended depth of 16384.

riccibruno marked an inline comment as done.Jan 7 2019, 5:32 AM
riccibruno added inline comments.
include/clang/AST/Expr.h
3030

But you can perhaps test this without doing any template instantiation.
The limit is "Direct and indirect base classes [16384]". Therefore you can
just construct 2^16 classes with macros which should not be recursive,
and then somehow construct a class with all of these classes as a base
(although this step is perhaps recursive).

riccibruno marked an inline comment as done.Jan 7 2019, 6:26 AM
riccibruno added inline comments.
include/clang/AST/Expr.h
3030

I meant obviously 2^14

This revision was automatically updated to reflect the committed changes.