r230255 - Only lower __builtin_setjmp / __builtin_longjmp to

[John McCall]

On Tue, Mar 10, 2015 at 7:16 PM, Joerg Sonnenberger <joerg at britannica.bec.de
> wrote:

> On Tue, Mar 10, 2015 at 07:03:16PM -0700, John McCall wrote:
> > On Tue, Mar 10, 2015 at 6:37 PM, Joerg Sonnenberger <
> joerg at britannica.bec.de
> > > wrote:
> >
> > > On Tue, Mar 10, 2015 at 05:44:00PM -0700, John McCall wrote:
> > > > I'm sorry, I missed your early request, and your response to my
> review.
> > > > I'm much more likely to respond quickly if you keep me as a
> recipient.
> > > >
> > > > I really would like you to diagnose this in Sema, please.
> > > Target-specific
> > > > restrictions are not new, especially on builtin functions.  But if
> you do
> > > > that, it's approved for merge.
> > >
> > > But Sema is too early, it breaks valid use cases that are never going
> to
> > > hit the backend at all. Consider clang --analyze or clang-modernize.
> > > Especially the latter is completely target independent, so it shouldn't
> > > get fail on code that is valid on one platform and only fails on
> another
> > > because of LLVM bugs.
> > >
> >
> > It's target-independent except for the thousands of ways that C code is
> not
> > target-independent.  Headers need to be in place and provide the right
> > declarations, hordes of warnings turn out differently based on type size,
> > printf format checking has target-specific logic, etc.
>
> A refactoring tool knows how to deal with many of those variations...


A refactoring tool generally needs to be smart enough to not accidentally
produce unportable code.  That is a different problem from actually being
able to function on a file that doesn't compile because it wasn't designed
to be portable in the first place.  And __builtin_setjmp / __builtin_longjmp
are not portable.


> > The way we (don't) implement them, __builtin_setjmp and __builtin_longjmp
> > are target-specific builtin functions, and they should be diagnosed along
> > with the rest of them.
>
> But that we don't implement them is a choice of CodeGen. Ideally, that
> wouldn't be needed if the IR intrinsics were cleanly separated, so this
> is really an implementation detail of the contract between Clang and
> LLVM IR and nothing about the semantic analysis of the source code. Why
> do you insist on violating the layering?


The contract between Clang and LLVM IR is completely unimportant
compared to the contract between the user and their compilation results.
Semantic analysis is supposed to be sufficient to identify all the problems
in a user's code.  We sometimes fall short of that, but we usually consider
those bugs, and we like to at least have good reasons for it.

There are many good reasons to raise this to the semantic-analysis layer.
There should probably be has a __has_feature check for this.  The size
of the buffer is effectively guaranteed, and a really good semantic analysis
layer would be checking that the buffer is at least that size.  We should
also be communicating that size to the user in some way instead of
forcing them to use a jmp_buf (which can be an order of magnitude
larger) for no particular reason.

If you want to keep arguing with me about this, go ahead, but I'm not
actually going to change my mind, so if you want this in either the
branch or trunk, you're going to need to move it to Sema.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150311/788f939c/attachment.html>