[llvm-commits] [PATCH] [MIPS] Avoid use of __builtin_unreachable() when compiling LLVM for MIPS.

Wed Jul 25 04:00:25 PDT 2012

> I don't think that's a quite accurate portrayal of the issue... it's more
> "our host compilers are full of bugs w.r.t. __builtin_unreachable". I think
> that's an issue w/ the host compiler, and folks should either A) find a
> better host compiler, or B) bootstrap and use a Clang-built tree.
> 
> If B doesn't work for some reason, you should work on that. ;] Patches very
> welcome there.
> I don't think its reasonable to hack around these broken compilers inside
> the LLVM codebase. Applying local patches to satisfy your build environment
> is a common reality.

Hi Chandler,

I believe our dedication to fully support LLVM is not questionable.
The key issue is that applying local patches easily works for someone who is
aware both of the issue and the patch. LLVM code gets pulled from the trunk
on different projects, and if we want other people not to have issues with
LLVM now, we need to find each project that uses LLVM, propose a patch, explain
it, and make sure it stays there after they sync to a newer version from LLVM
trunk. Add to that all valid points about testing that Reed has listed
previously.

Today, whoever checks-out code from LLVM trunk and uses recent versions of the
common crosstoolchain (and not any specific toolchain that none else would be
using) for MIPS architecture is likely to end up with a code that will have
this issue. Adding a change that is literally a two-liner to prevent this
awkward case seems reasonable to me. If we do not add a change, then we have a
code base that will *not* work for MIPS. I do not see how the latter is a
better solution.

Having said this, fixing the host compiler is - no discussion here - the best
place to correct this, and we are already working on this. However, having in
mind toolchain release cycles, toolchain distribution and everything else, we
need LLVM code to be aware of the issue in the meantime. We are not giving up
on builtin_unreachable for MIPS for good. We just need to temporarily disable
it in LLVM code.

Petar

________________________________________
From: Jovanovic, Petar
Sent: Wednesday, July 25, 2012 4:29 AM
To: Eric Christopher
Cc: llvm-commits at cs.uiuc.edu; benny.kra at googlemail.com
Subject: RE: [llvm-commits] [PATCH] [MIPS] Avoid use of __builtin_unreachable() when compiling LLVM for MIPS.

Hi Eric,

as I mentioned in the previous email, the bug report I pasted link for was the original root cause. There seem to be more issues caused by __builtin_unreachable, and toolchains that have a fix for that one (for instance latest Google 4.6.x toolchain for Android) still have issues with __builtin_unreachable for MIPS.

We will inevitably resolve all these issues (we are debugging the latest occurrence now), but until that point is reached, we need to compile LLVM correctly.

Petar
________________________________________
From: Eric Christopher [echristo at apple.com]
Sent: Wednesday, July 25, 2012 4:10 AM
To: Jovanovic, Petar
Cc: llvm-commits at cs.uiuc.edu; benny.kra at googlemail.com
Subject: Re: [llvm-commits] [PATCH] [MIPS] Avoid use of __builtin_unreachable() when compiling LLVM for MIPS.

Hi Petar,

It looks like it was fixed in gcc 4.5.4 according to that bug report and I'm loath
to turn off __builtin_unreachable for mips unconditionally because of a
bug that's known fixed in multiple released versions.

We could put it down as a known bad revision on the website for that architecture.

-eric

On Jul 24, 2012, at 2:48 PM, "Jovanovic, Petar" <petarj at mips.com> wrote:

> Hi Eric,
>
> I have just updated the patch to include the comment for the change.
>
> The issue is that code for some switch statements with __builtin_unreachable
> is not correctly generated. Some cases miss, and the code is wrong. The wrong
> code resulted in wrong values and eventually in null-pointer dereference and
> code crash. The issue has been found in Renderscript examples in Android that
> crashed due to incorrect llvm library code.
>
> Regards,
> Petar
> ________________________________________
> From: Eric Christopher [echristo at apple.com]
> Sent: Tuesday, July 24, 2012 10:34 PM
> To: Jovanovic, Petar
> Cc: llvm-commits at cs.uiuc.edu; Benjamin Kramer
> Subject: Re: [llvm-commits] [PATCH] [MIPS] Avoid use of __builtin_unreachable() when compiling LLVM for MIPS.
>
> I'm not seeing any update in the code as to why you're working around it (which was Benjamin's request), and the comment that you made in your reply also doesn't state what the bug was other than "issue".
>
> -eric
>
> On Jul 24, 2012, at 11:46 AM, "Jovanovic, Petar" <petarj at mips.com> wrote:
>
>> PTAL
>>
>> -----Original Message-----
>> From: Petar Jovanovic [mailto:petar.jovanovic at rt-rk.com]
>> Sent: Thursday, April 12, 2012 3:15 AM
>> To: 'Benjamin Kramer'
>> Cc: 'llvm-commits at cs.uiuc.edu'
>> Subject: RE: [llvm-commits] [PATCH] [MIPS] Avoid use of __builtin_unreachable() when compiling LLVM for MIPS.
>>
>> Any additional comments or concerns about this one?
>>
>> Regards,
>> Petar
>>
>> -----Original Message-----
>> From: Petar Jovanovic [mailto:petar.jovanovic at rt-rk.com]
>> Sent: Friday, March 23, 2012 7:15 PM
>> To: 'Benjamin Kramer'
>> Cc: 'llvm-commits at cs.uiuc.edu'
>> Subject: RE: [llvm-commits] [PATCH] [MIPS] Avoid use of __builtin_unreachable() when compiling LLVM for MIPS.
>>
>> The issue was first found on the latest cross gcc from Mentor [gcc version 4.5.2 (Sourcery CodeBench Lite 2011.09-75)], and since __builtin_unreachable() is available as of gcc 4.5, it seemed to me that the scope is already narrow enough. I can also confirm that the bug can be reproduced with native gcc (GCC) 4.5.1 and gcc (GCC) 4.5.3 , and I am actually not aware of gcc version that supports __builtin_unreachable in which the bug is not present.
>>
>> The bug was discovered three days ago in Android builds for MIPS. The function where this issue was discovered is:
>> unsigned TargetData::getAlignment(Type *Ty, bool abi_or_pref)
>>
>> at lib\Target\TargetData.cpp.
>>
>> Petar
>>
>> -----Original Message-----
>> From: Benjamin Kramer [mailto:benny.kra at googlemail.com]
>> Sent: Friday, March 23, 2012 12:57 AM
>> To: Petar Jovanovic
>> Cc: llvm-commits at cs.uiuc.edu; Petar Jovanovic
>> Subject: Re: [llvm-commits] [PATCH] [MIPS] Avoid use of __builtin_unreachable() when compiling LLVM for MIPS.
>>
>> On 23.03.2012, at 00:20, Petar Jovanovic <petar.jovanovic at rt-rk.com> wrote:
>>
>>> There is a GCC bug which can be triggered inside of LLVM library. The -O2 opt
>>> does not work well with __builtin_unreachable(), so we are avoiding it inside
>>> of LLVM. The issue seems to be caused by 'freorder-blocks' optimization.
>>
>> Patches working around compiler bugs are generally okay, but please
>>
>> - Add a comment on why you're blacklisting MIPS here.
>> - Make the blacklist as narrow as possible, i.e. check for the
>> specific version of GCC that has this bug.
>>
>> - Ben
>>>
>>> Signed-off-by: Petar Jovanovic <petarj at mips.com>
>>> ---
>>> include/llvm/Support/Compiler.h |    2 ++
>>> 1 files changed, 2 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/include/llvm/Support/Compiler.h b/include/llvm/Support/Compiler.h
>>> index 9c5e7ec..b5a086a 100644
>>> --- a/include/llvm/Support/Compiler.h
>>> +++ b/include/llvm/Support/Compiler.h
>>> @@ -144,9 +144,11 @@
>>> // LLVM_BUILTIN_UNREACHABLE - On compilers which support it, expands
>>> // to an expression which states that it is undefined behavior for the
>>> // compiler to reach this point.  Otherwise is not defined.
>>> +#if !defined(__mips__)
>>> #if defined(__clang__) || (__GNUC__ > 4) \
>>> || (__GNUC__ == 4 && __GNUC_MINOR__ >= 5)
>>> # define LLVM_BUILTIN_UNREACHABLE __builtin_unreachable()
>>> #endif
>>> +#endif
>>>
>>> #endif
>>> --
>>> 1.7.5.4
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
> <disable-builtin-unreachable.diff>