r230255 - Only lower __builtin_setjmp / __builtin_longjmp to

[John McCall]

On Mar 2, 2015, at 11:22 PM, Hal Finkel <hfinkel at anl.gov> wrote:
> 
> ----- Original Message -----
>> From: "Joerg Sonnenberger" <joerg at britannica.bec.de>
>> To: "John McCall" <rjmccall at gmail.com>
>> Cc: hans at hanshq.net, "cfe-commits" <cfe-commits at cs.uiuc.edu>
>> Sent: Tuesday, March 3, 2015 1:07:20 AM
>> Subject: Re: r230255 - Only lower __builtin_setjmp / __builtin_longjmp to
>> 
>>> On Mon, Mar 02, 2015 at 06:02:32PM -0800, John McCall wrote:
>>> This patch is pretty scary.  __builtin_setjmp/longjmp are
>>> definitely not
>>> just libc functions with a __builtin_ prefix attached.  They do not
>>> interoperate with setjmp/longjmp and expect a significantly smaller
>>> buffer,
>>> so silently rewriting them to setjmp/longjmp is ABI-breaking.  This
>>> might
>>> fix Ruby, but only if Ruby is actually passing a full jmp_buf, and
>>> only if
>>> everything that does a __builtin_setjmp/__builtin_longjmp is
>>> recompiled in
>>> a way that does the rewrite.  I'm very concerned about this
>>> introducing ABI
>>> problems for a Clang-compiled Ruby with GCC-compiled extensions or
>>> vice-versa.  FWIW, Ruby seems to already have target-specific
>>> configuration
>>> logic for when to use them.
>> 
>> Ruby has no target-specific configuration.

Literally the first google result for "Ruby __builtin_setjmp" is a core commit turning it off for a target.  Am I misunderstanding something?

GCC implements this builtin with much more generality than we do, probably because GCC predates widespread adoption of libUnwind and we don't.  Hal's comment about not trying to match GCC on PPC aside (and I find that comment pretty troubling!), my understanding of the general intent of this builtin is that's for implementing extremely lightweight context-saving (on the assumption that the platform doesn't have that many callee-save registers; it would probably be awful on e.g. AArch64) and that the amount of state it saves is so small (PC, SP, FP if it's not rederivable from SP) that there's no reason [i]not[/i] to match GCC.

The fact that it's poorly documented and poorly implemented doesn't mean that breaking compatibility arbitrarily is acceptable.  It might be an unfortunate and obscure bit of ABI, but absent other information, I consider it ABI.  So I am not inclined to accept this, for trunk or for the branch.  I think it would be a much more modest and reasonable fix to just implement the builtin correctly in the ARM backend and wherever else you see the need.  Presumably Ruby is using this because they actually care about the performance.

But if you can get the LLVM list to agree with Hal that compatibility (even across compiler versions) is a non-goal, and you can show me more convincingly that e.g. Ruby only uses this in their core implementation and it is not used directly in the compiled code of native gems, then I am willing to change my mind.

John.

>> if that works, it picks them. Given that the current __builtin_setjmp
>> /
>> __builtin_longjmp code is completely broken unless the backend has
>> explicit logic for that, the configure test in ruby will pass on all
>> ARM
>> platforms except Darwin (where is might hit an assert or not).
>> 
>> No, they are not libc functions with a prefix as they are allowed to
>> be
>> lowered to a different format. The builtin documentation is scarce at
>> best. It is a typically badly designed GCC builtin. There is a half
>> sentence in gccint.info about the buffer being 5 longs long, but even
>> the test suite is not consistent in that regard. It looks like a
>> completely messed up state.
>> 
>> Concerning ABI use: AFAICT, Ruby is the only thing outside an EH
>> implementation which actually tries to use them. It provides a
>> correct
>> jmpbuf_t, so it gets a large enough buffer.
> 
> Generically, these builtins are used in lightweight task-switching runtimes (that's why I implemented them in the PowerPC backend; it had nothing to do with EH) -- they're an order of magnitude faster than the function calls.
> 
> Also, as far as the PowerPC implementation is concerned, gcc compatibility is a non-goal. These are compiler builtins, and I don't consider them to be part of the ABI.
> 
> FWIW, I think that defaulting to the library implementation is reasonable, it does provide a valid (albeit slow) implementation.
> 
> -Hal
> 
>> 
>>> Therefore, if we don't actually consistently support these builtins
>>> in the
>>> backend in a GCC-compatible way (quite plausible), I would be much
>>> more
>>> comfortable diagnosing that than silently rewriting them to
>>> setjmp/longjmp,
>>> unless there are platforms where GCC does actually rewrite them.
>>> Have you
>>> done that investigation?
>> 
>> I have been discussing the approach and the lowering to a
>> known-to-work
>> sj/lj seemed the least broken approach. I don't care enough about not
>> just bailing out in the frontend either. It is hard to tell what the
>> many different GCC configs are doing, I gave up.
>> 
>> Joerg
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
> 
> -- 
> Hal Finkel
> Assistant Computational Scientist
> Leadership Computing Facility
> Argonne National Laboratory