-
Notifications
You must be signed in to change notification settings - Fork 345
[BoundsSafety] Allow evaluating the same CLE in a bounds-check #11175
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
base: next
Are you sure you want to change the base?
[BoundsSafety] Allow evaluating the same CLE in a bounds-check #11175
Conversation
Are we sure this isn't something that should be wrapped in OpaqueValueExpr to allow sharing the AST node? |
TBH, I'm not sure. This is caused by building a bounds-check in We create the bounds-check condition first, which causes the bug, and then wrap it in BoundsCheckExpr/MaterializeSequenceExpr. If anyone has a better idea how to solve it, please comment. |
5a70759
to
410016e
Compare
I was imagining in general that if you are creating new AST nodes and want to reuse some nodes, you need to make clones of them. But I'm not sure if this is actually required in clang development. |
Ah, I didn't know that |
Earlier in the function we scan all the arguments passed in the function call, and wrap them in OVEs. Is this not done for the compound literal? Or do we somehow peel the OVE off later? Perhaps the explicit cast is related. |
The compound literal is wrapped in OVE at this point. For context: frame #29: 0x0000000108250c78 clang`clang::Expr::EvaluateAsInt(this=0x000000015093b018, Result=0x000000016fdebdd8, Ctx=0x0000000150830800, AllowSideEffects=SE_NoSideEffects, InConstantContext=false) const at ExprConstant.cpp:17556:10
* frame #30: 0x00000001069b169c clang`clang::Sema::CheckLogicalOperands(this=0x000000015090c000, LHS=0x000000016fdec558, RHS=0x000000016fdec550, Loc=(ID = 43), Opc=BO_LAnd) at SemaExpr.cpp:16428:20
frame #31: 0x00000001069af41c clang`clang::Sema::CreateBuiltinBinOp(this=0x000000015090c000, OpLoc=(ID = 43), Opc=BO_LAnd, LHSExpr=0x000000015093afc8, RHSExpr=0x000000015093b018, ForFoldExpression=false) at SemaExpr.cpp:18282:16
frame #32: 0x00000001069d1bf4 clang`clang::BoundsCheckBuilder::BuildLEChecks(S=0x000000015090c000, Loc=(ID = 43), Bounds=ArrayRef<clang::Expr *> @ 0x000000016fdee840, OVEs=0x000000016fdeeec0) at SemaExpr.cpp:26301:30
frame #33: 0x00000001069cffc4 clang`clang::BoundsCheckBuilder::Build(this=0x000000016fdeecc0, S=0x000000015090c000, GuardedValue=0x000000015093ad48) at SemaExpr.cpp:26250:37
frame #34: 0x0000000106982c14 clang`clang::Sema::ActOnBoundsSafetyCall(this=0x000000015090c000, Call=(Value = 5646822728)) at SemaExpr.cpp:25737:30
(lldb) f 30
frame #30: 0x00000001069b169c clang`clang::Sema::CheckLogicalOperands(this=0x000000015090c000, LHS=0x000000016fdec558, RHS=0x000000016fdec550, Loc=(ID = 43), Opc=BO_LAnd) at SemaExpr.cpp:16428:20
16425 // happened to fold to true/false) then warn.
16426 // Parens on the RHS are ignored.
16427 Expr::EvalResult EVResult;
-> 16428 if (RHS.get()->EvaluateAsInt(EVResult, Context)) {
16429 llvm::APSInt Result = EVResult.Val.getInt();
16430 if ((getLangOpts().CPlusPlus && !RHS.get()->getType()->isBooleanType() &&
16431 !RHS.get()->getExprLoc().isMacroID()) ||
(lldb) p RHS.get()->dumpColor()
BinaryOperator 0x15093b018 'int' '<='
|-ImplicitCastExpr 0x15093afe8 'char *' <BoundsSafetyPointerCast>
| `-GetBoundExpr 0x15093ae38 'char *__bidi_indexable' lower
| `-OpaqueValueExpr 0x15093adf8 'char *__bidi_indexable'
| `-ImplicitCastExpr 0x15093adb0 'char *__bidi_indexable' <ArrayToPointerDecay>
| `-CompoundLiteralExpr 0x15093ac38 'char[3]' lvalue
| `-InitListExpr 0x15093ab98 'char[3]'
| |-ImplicitCastExpr 0x15093abf0 'char' <IntegralCast>
| | `-CharacterLiteral 0x15093aae8 'int' 65
| |-ImplicitCastExpr 0x15093ac08 'char' <IntegralCast>
| | `-CharacterLiteral 0x15093ab00 'int' 66
| `-ImplicitCastExpr 0x15093ac20 'char' <IntegralCast>
| `-CharacterLiteral 0x15093ab18 'int' 67
`-ImplicitCastExpr 0x15093b000 'char *' <BoundsSafetyPointerCast>
`-OpaqueValueExpr 0x15093adf8 'char *__bidi_indexable'
`-ImplicitCastExpr 0x15093adb0 'char *__bidi_indexable' <ArrayToPointerDecay>
`-CompoundLiteralExpr 0x15093ac38 'char[3]' lvalue
`-InitListExpr 0x15093ab98 'char[3]'
|-ImplicitCastExpr 0x15093abf0 'char' <IntegralCast>
| `-CharacterLiteral 0x15093aae8 'int' 65
|-ImplicitCastExpr 0x15093ac08 'char' <IntegralCast>
| `-CharacterLiteral 0x15093ab00 'int' 66
`-ImplicitCastExpr 0x15093ac20 'char' <IntegralCast>
`-CharacterLiteral 0x15093ab18 'int' 67 The issue here is that those
|
Uhh, not sure if my patch is even correct: void foo(char tag[3]);
void bar(void) {
char c = 'A';
foo((char[3]){c++, 'B', 'C'});
} |
TBF, this constant evaluation is used to diagnose: return x && 0; // expected-warning {{use of logical '&&' with constant operand}} \
We could disable this if we are inside of bounds-check condition and call it a day... |
This feels like a bug imo: |
The |
This sounds reasonable to me. |
Ahh that explains it, I didn't even notice that |
BoundsSafety can reuse the same CompoundLiteralExpr when emitting a bounds-check condition. This change disables constant-eval when inside of CompoundLiteralExpr. rdar://157033241
410016e
to
b0e93f1
Compare
It turns out that we also invoke const-eval in codegen: * frame #4: 0x00000001081ffd40 clang`(anonymous namespace)::CallStackFrame::createLocal(this=0x000000016fdeee08, Base=LValueBase @ 0x000000016fdea9c8, Key=0x0000000123842238, T=QualType @ 0x000000016fdea9c0, Scope=Block) at ExprConstant.cpp:2148:3
frame #5: 0x0000000108286844 clang`clang::APValue& (anonymous namespace)::CallStackFrame::createTemporary<clang::CompoundLiteralExpr>(this=0x000000016fdeee08, Key=0x0000000123842238, T=QualType @ 0x000000016fdeaa60, Scope=Block, LV=0x000000016fdec2c0) at ExprConstant.cpp:2128:10
frame #6: 0x00000001081d6678 clang`(anonymous namespace)::LValueExprEvaluator::VisitCompoundLiteralExpr(this=0x000000016fdeac38, E=0x0000000123842238) at ExprConstant.cpp:9445:30
...
frame #40: 0x00000001081c55b4 clang`clang::Expr::EvaluateAsInt(this=0x0000000123842638, Result=0x000000016fdef1c8, Ctx=0x000000012001de00, AllowSideEffects=SE_NoSideEffects, InConstantContext=false) const at ExprConstant.cpp:17945:10
frame #41: 0x0000000101d5f078 clang`clang::CodeGen::CodeGenFunction::ConstantFoldsToSimpleInteger(this=0x000000016fdf19c8, Cond=0x0000000123842638, ResultInt=0x000000016fdef248, AllowLabels=false) at CodeGenFunction.cpp:1827:14
frame #42: 0x0000000101d5efa0 clang`clang::CodeGen::CodeGenFunction::ConstantFoldsToSimpleInteger(this=0x000000016fdf19c8, Cond=0x0000000123842638, ResultBool=0x000000016fdef6e7, AllowLabels=false) at CodeGenFunction.cpp:1811:8
frame #43: 0x00000001019ef778 clang`(anonymous namespace)::ScalarExprEmitter::VisitBinLAnd(this=0x000000016fdef9c8, E=0x0000000123842658) at CGExprScalar.cpp:5454:11
frame #44: 0x00000001019ecae8 clang`clang::StmtVisitorBase<std::__1::add_pointer, (anonymous namespace)::ScalarExprEmitter, llvm::Value*>::Visit(this=0x000000016fdef9c8, S=0x0000000123842658) at StmtVisitor.h:72:26
frame #45: 0x00000001019e3914 clang`(anonymous namespace)::ScalarExprEmitter::Visit(this=0x000000016fdef9c8, E=0x0000000123842658) at CGExprScalar.cpp:456:52
frame #46: 0x00000001019e3708 clang`clang::CodeGen::CodeGenFunction::EmitScalarExpr(this=0x000000016fdf19c8, E=0x0000000123842658, IgnoreResultAssign=false) at CGExprScalar.cpp:6064:8
frame #47: 0x000000010190bb68 clang`clang::CodeGen::CodeGenFunction::EvaluateExprAsBool(this=0x000000016fdf19c8, E=0x0000000123842658) at CGExpr.cpp:471:33
frame #48: 0x0000000101915230 clang`clang::CodeGen::CodeGenFunction::EmitBoundsSafetyTrapCheck(this=0x000000016fdf19c8, Cond=0x0000000123842658, kind=BNS_TRAP_GENERAL) at CGExpr.cpp:1656:26
frame #49: 0x00000001019f7c84 clang`(anonymous namespace)::ScalarExprEmitter::VisitBoundsCheckExpr(this=0x000000016fdf0588, BCE=0x0000000123842678) at CGExprScalar.cpp:1026:11
frame #50: 0x00000001019ed7c0 clang`clang::StmtVisitorBase<std::__1::add_pointer, (anonymous namespace)::ScalarExprEmitter, llvm::Value*>::Visit(this=0x000000016fdf0588, S=0x0000000123842678) at StmtNodes.inc:822:1
frame #51: 0x00000001019e3914 clang`(anonymous namespace)::ScalarExprEmitter::Visit(this=0x000000016fdf0588, E=0x0000000123842678) at CGExprScalar.cpp:456:52
frame #52: 0x00000001019f4f20 clang`(anonymous namespace)::ScalarExprEmitter::VisitMaterializeSequenceExpr(this=0x000000016fdf0588, MSE=0x0000000123842698) at CGExprScalar.cpp:1076:12 Do we also want to disable the const-eval in codegen when emitting bounds-check? We will probably run into more issues like this. Does anyone have any better idea? |
Perhaps we could temporarily push a new call frame to |
For codegen, it's a bit different problem because then we have already constructed the bounding nodes for OVEs - |
BoundsSafety can reuse the same CompoundLiteralExpr when emitting a bounds-check condition. This change allows evaluating the same CLE in LValueExprEvaluator::VisitCompoundLiteralExpr, and when it was previously evaluated, we reset the state and evaluate it again.
rdar://157033241