Page MenuHomePhabricator

[AST] CastExpr: BasePathSize is not large enough.
ClosedPublic

Authored by lebedev.ri on Jul 31 2018, 4:10 AM.

Details

Summary

rC337815 / D49508 had to cannibalize one bit of CastExprBitfields::BasePathSize in order to squeeze PartOfExplicitCast boolean.
That reduced the maximal value of PartOfExplicitCast from 9 bits (~512) down to 8 bits (~256).
Apparently, that mattered. Too bad there weren't any tests.
It caused PR38356.

So we need to increase PartOfExplicitCast back at least to 9 bits, or a bit more.
For obvious reasons, we can't do that in CastExprBitfields - that would blow up the size of every Expr.
So we need to either just add a variable into the CastExpr (as done here),
or use llvm::TrailingObjects. The latter does not seem to be straight-forward.
Perhaps, that needs to be done not for the CastExpr itself, but for all of it's final children.

Diff Detail

Repository
rC Clang

Event Timeline

lebedev.ri created this revision.Jul 31 2018, 4:10 AM

Its not that it needs to be 'at least 9 bits', IMO 9 bits was too small as well.

What I'd prefer to this solution is similar to what John McCall said on PR38356: Add the size to the trailing allocated space. Check out the 'create' methods for all of these, and make the first sizeof(void*) bytes be the size. I'd also suggest using 1 bit in CastExprBits to be a 'is base path empty', so that you don't have to allocate for it at all.

include/clang/AST/Expr.h
2796

The comment there is wrong. It was wrong at 9 bits as well, since it cannot handle our implimits. I'd like to see if you can find where the implimit for direct/indirect bases is stored, and write a static-assert for it here.

test/CodeGenCXX/castexpr-basepathsize-threshold.cpp
1

First, most of this test can be further simplified. Second, I'd like to see a test that actually tests the limit without hitting the recursive template limit.

20

Shouldn't we cover 16k?

lebedev.ri marked an inline comment as done.
  • Store/prepend it into llvm::TrailingObjects<>
  • Use unsigned short
  • Assert that the storage is at least large enough to be compliant with implimits
  • Don't allocate even that unsigned short unless the path is not empty (use one bit in CastExprBits)
test/CodeGenCXX/castexpr-basepathsize-threshold.cpp
1

First, most of this test can be further simplified.

OK? This is what creduce gave me. But yes, i know it has some issues with templates.

Second, I'd like to see a test that actually tests the limit without hitting the recursive template limit.

But that is true already. *this* test does not hit the recursive template limit.

20

No, for me 4096 already crashes elsewhere.

erichkeane added inline comments.Jul 31 2018, 10:19 AM
include/clang/AST/Expr.h
2791

I'll let @rjmccall comment here, but I think I'd be willing to give up 2 more bytes here and just go with unsigned int. It has the advantage of likely never being an issue again (since 4 billion bases is not an issue).

We might be paying for those 2 ANYWAY with alignment as well, right?

2799

Looking back up, I doubt the need for this either, since we have path_empty and path_size below.

2804

Why is this a reference? This seems odd... It seems that the const-cast magic above shouldn't be necessary either.

2806

This is a super weird way to do this. I really am not a fan of giving a reference to that spot instead of just setting it here.

2841

Is this necessary? Shouldn't path_empty just do this?

test/CodeGenCXX/castexpr-basepathsize-threshold.cpp
1

Looking again, it is probably reduced sufficiently. The cast and the recurse-depth is the only things that are important, so I guess I'm OK with this.

erichkeane added inline comments.Jul 31 2018, 10:45 AM
include/clang/AST/Expr.h
2804

Looking more into this, I think the reference here is an 'equal of two evils'. I can't catch myself really preferring one way over the other.

2807

This assert shouldn't be necessary. I think setBasePathSize could just take BasePathSizeTy.

lebedev.ri marked 6 inline comments as done.

Address some of @erichkeane review notes.

include/clang/AST/Expr.h
2791

Yes, i do expect that we are paying for 2 bytes in padding already.

2804

I'll make it a pointer and replace BasePathSizeTy BasePathSize() const with path_size().
I don't really think it would be good to get rid of this pointer/reference, since then
we'd have more than one function doing what CastExpr::BasePathSizeTy *CastExpr::BasePathSize() does now.

rjmccall accepted this revision.Jul 31 2018, 2:02 PM

LGTM.

include/clang/AST/Expr.h
2791

Yeah, we could really use a size_t here without loss, but an unsigned is fine.

This revision is now accepted and ready to land.Jul 31 2018, 2:02 PM

Drop pointless cast of zero.

LGTM.

Thank you for the review.
I'll land in ~+9 hours or so, since i did not hear from @erichkeane back yet..

Closed by commit rC338489: [AST] CastExpr: BasePathSize is not large enough. (authored by lebedevri, committed by ). · Explain WhyJul 31 2018, 11:06 PM
This revision was automatically updated to reflect the committed changes.

@lebedev.ri @erichkeane The test fails for me on macOS whenever asan and ubsan are both enabled.
The failure is stack overflow at stack frame 943 (? maybe asan usage enforces lower stack size?)

@lebedev.ri @erichkeane The test fails for me on macOS whenever asan and ubsan are both enabled.
The failure is stack overflow at stack frame 943

(? maybe asan usage enforces lower stack size?)

Or higher stack usage.
But yes, sounds like it.

@lebedev.ri yeah ASAN is making stack frame size larger.

It seems @vsapsai is working on disabling this test under ASAN.

Disabling test in D54132.