This is an archive of the discontinued LLVM Phabricator instance.

Fix PR31843: Clang-4.0 crashes/assert while evaluating __builtin_object_size
ClosedPublic

Authored by george.burgess.iv on Feb 2 2017, 12:33 PM.

Details

Summary

We're currently using a special EvaluationMode to determine whether
we're OK with invalid base expressions during objectsize evaluation.
Using it to figure out how we handle UB/etc. is fine, but I think it's
too far-reaching to use for checking whether we're OK with an invalid
base expression. This is because an EvaluationMode applies by default to
all subexpressions of the expression we're evaluating, which causes
issues like in https://llvm.org/bugs/show_bug.cgi?id=31843 .

What I think we actually want to do is allow this relaxed behavior only
for "top-level" member/pointer expressions. In other words, we should
only allow it when we're evaluating the top-level pointers/lvalues
involved in an expression. As soon as we need to evaluate something else
(an int, ...), we should drop these relaxed rules and go back to
stricter evaluation. For example, in:

foo(whatever->a ? b() : 1)->bar->baz.quux

We don't care if whatever->a is evaluated using these relaxed rules,
since the only invalid base that's actually useful to the objectsize
evaluator is foo(...)->bar.

...As a lower level issue, one idea I had to make this less awkward was
to just define EvaluatePointer(const Expr *, LValue&, EvalInfo&) as a
method on LValueExprEvaluator/PointerExprEvaluator that called
::EvaluatePointer with the right fourth arg, but that seemed a bit too
subtle to me. Happy to swap to it if you think it's better.

Finally, if we make this change, ISTM we can just replace OffsetFold
with ConstantFold. I'd rather to that in another patch, since whatever
we choose to do here gets put into 4.0.


With all of that said, if we want the minimal fix for PR31843, it's
adding an InvalidBase check or two to Evaluate. My concern with that
approach is that we'd just end up playing whack-a-bug. It's possible
that this fix puts us in a similar situation, but I'm honestly at a loss
for a better approach. ¯\_(ツ)_/¯ (...And I'd kinda prefer to play
whack-a-bug with "clang could do a better job in this case" reports,
rather than "clang crashes on my code" reports.)

Diff Detail

Repository
rL LLVM

Event Timeline

My concern with that approach is that we'd just end up playing whack-a-bug. It's possible that this fix puts us in a similar situation, but I'm honestly at a loss for a better approach. ¯\_(ツ)_/¯ (...And I'd kinda prefer to play whack-a-bug with "clang could do a better job in this case" reports, rather than "clang crashes on my code" reports.)

Agree, thanks :)

This patch LGTM but I have little confidence in myself in this area of the compiler, so I rather have someone else stamping it!

Ping? This is hitting 4.0

End of week ping :)

dexonsmith added a subscriber: dexonsmith.

+Nuno, who worked on objectsize in the past.

rsmith accepted this revision.Feb 10 2017, 2:36 PM

LGTM, I think this is also OK for Clang 4 if Hans is willing to take it.

lib/AST/ExprConstant.cpp
607–612 ↗(On Diff #86870)

[nit] "We", "We", "Then" should not be capitalized, and please remove blank line prior to "Then we [...]"

This revision is now accepted and ready to land.Feb 10 2017, 2:36 PM
george.burgess.iv marked an inline comment as done.Feb 10 2017, 3:03 PM

Thanks!

This revision was automatically updated to reflect the committed changes.