rjmccall added a comment.
In D137381#3953178 <https://reviews.llvm.org/D137381#3953178>, @lebedev.ri
wrote:
> ping. We need to get this going, reminder, this was caught because it caused
> a visible miscompilation.
I'm sorry, I caught COVID-19 during the LLVM conference and have not had a lot
of energy recently, plus last week was the Thanksgiving holiday in the US.
Please don't describe this as a miscompilation. It may not match what you want
as a user, but what the the compiler is doing is not incorrect. What you're
doing is adding a mitigation to try to protect against a source of UB that
recently affected you, which is commendable but doesn't have the same level of
urgency.
================
Comment at: clang/lib/CodeGen/CGCall.cpp:5402-5411
+ if (SanOpts.has(SanitizerKind::ExceptionEscape) &&
+ ExceptionEscapeUBLastInvokeSrcLoc) {
+ llvm::Constant *CheckSourceLocation = EmitCheckSourceLocation(Loc);
+ Builder.CreateStore(
+ CheckSourceLocation,
+ Address(ExceptionEscapeUBLastInvokeSrcLoc,
+ CheckSourceLocation->getType(),
----------------
lebedev.ri wrote:
> lebedev.ri wrote:
> > @rjmccall
> > So there are two issues:
> > 1. `getInvokeDest()` isn't necessarily called just before creating an
> > `invoke`, see e.g. `-EHa` handling in `CodeGenFunction::PopCleanupBlock()`
> > 2. Even if we ignore that, we need to do this for *every* `invoke`, not
> > just those going to the our UB landing pad, consider:
> > https://godbolt.org/z/qTeKor41a <- the invoke leads to a normal landing
> > pad, yet we immediately rethrow the just-caught exception, and now end up
> > in the UB landing pad.
> >
> > So i'm not really seeing an alternative path here?
> @rjmccall do you agree with my reasoning here?
> Perhaps there is some other solution that i'm not seeing,
> other than not having the source location?
I think there are four interesting questions posed by your example.
The first is whether we should make an effort to report the original source
location for the call to `maybe_throws` instead of the rethrow location. To
me, the answer is no, and the main reason is that the user is explicitly
acknowledging the possibility of an exception and is (probably) trying to
handle it. That means that the proximate source of programmer error is now
that their attempt to handle it failed, not that an exception was raised in the
first place. That might seem weird for your example specifically,
The second is what source location we should report for invokes that don't have
an obvious source location. The answer is that I think we should always try to
have a source location, even if it might be an imperfect one. Synthesized code
almost always has a source location that triggered the synthesis. Cleanups are
typically associated with some variable or temporary that itself should have a
source location. Even things like `__cxa_end_catch` are associated with the
`catch` clause. Hopefully UBSan has a flexible enough schema in how source
locations are expressed to it for us to communicate these things down to the
runtime, like "synthesized code at Foo.cpp:18" or "cleanup for variable at
Bar.cpp:107".
The third is the more general question of whether we need to store a source
location before every `invoke`. To ensure that this variable is meaningfully
initialized, we need to store a source location before every invoke that can
lead to a UB scope. But the variable is stateful, so it's equally important
that we not emit stores that might overwrite the source location that we
actually want to report. For example, we want the source location reported in
your example to be the source location for the `throw;`, not something
associated with the `__cxa_end_catch` cleanup. This basically boils down to
not doing stores within a landing pad before it enters a `catch` handler.
Fortunately, landing pad code like that should always be in a terminate scope,
so I think you can just look at the current EH stack and see whether the
landing pad can possibly reach a UB scope. (You might need some
save-and-restore logic around `@finally` blocks in ObjC.)
The final question is whether we need to emit cleanups on paths that lead to UB
scopes at all. In the simplest cases, I think we don't. The standard says
that it's implementation-specified whether the stack is unwound on an exception
path that leads to termination. That rule doesn't technically apply here, but
only because we're dealing with a language extension that makes unwinding UB in
the first place. Since we have room to choose the semantics, the standard
gives us pretty strong cover to be just as aggressive about skipping cleanups
in contexts where throwing is UB as it is in contexts where throwing
terminates. That, in turn, means that the landing pad in these contexts will
usually just be a UB scope with no cleanups in it. So even if we did need to
worry about cleanups overwriting the source location, it usually won't be
possible because we won't be running cleanups. In fact, if there's a catch-all
or a terminate scope, the UB scope won't be reachable, so the only situation
where we have to worry about cleanups being run before we reach a UB scope is
if there are cleanups inside a *partial* catch clause, like `catch (const
std::exception &e)`. And in principle we have the information up front from
the EH selector to know whether we're going to end up in a catch handler, so we
could actually just check immediately at the start of the landing pad and then
trigger UBSan.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D137381/new/
https://reviews.llvm.org/D137381
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
