[llvm] r174336 - [Stack Alignment] emit warning instead of a hard error

Wed Feb 6 22:39:24 PST 2013

I think I've been on both sides of this argument at different times. So far most of the discussion has been hypothetical, but we now have some data.  We're seeing this error trigger in a fair number of cases during an Apple build. I've gone through and looked at the causes, and they fall into two categories:

1. Confusion about the alignment being specified in bytes, not bits.  E.G.: Someone wanted a 4-byte aligned buffer and wrote "aligned(32)".

2. Attempts to optimize or at least stabilize performance, e.g., by forcing the alignment of a data structure to match the cache line size.  In many, if not all of these cases, the alignment is specified on a data structure that is not primarily allocated on the stack.  But, if the code has any instance of that data on the stack, it will trigger the error.

Just to be clear, none of these cases that I saw were real errors.  They were all cases where the code would have run just fine without the requested alignment.  Reporting these cases as hard errors is not adding any value.

Regardless of that, I'm going to ask that this change and the error check be reverted for now -- not because of the warning vs. error issue, but simply because it's an absolutely terrible diagnostic.  We're not providing any source location information, so all you get now is a message that says there was an error.  You don't even get a line number.

The right way to implement this check is in the frontend.  Given the experience we've had with making this an error in an Apple build, I think it should be a warning.  Anyone who is playing games with the low bits of pointers can choose to promote that warning to an error when building their code.

On Feb 5, 2013, at 5:04 PM, Manman Ren <mren at apple.com> wrote:

> 
> On Feb 5, 2013, at 11:03 AM, Eric Christopher <echristo at gmail.com> wrote:
> 
>> Well, I think we've gone from a compile time error for code like this:
>> 
>> void foo () {
>>   my_32byte_align_requested_struct a;
>> 
>>    <insert inline asm that depends upon 32-byte alignment>
>> 
>> }
>> 
>> to a run time crash.
> 
> That is true, with this patch, we are going from a compile time error with r172027 to a run time crash with warning, for the above case.
> If the source code does not depend on the 32-byte alignment, we are going from a compile time error to a warning.
> 
> Before r172027, it is a run time crash without any warning.
> 
> Let me know what you think :]
> 
> Thanks,
> Manman
>  
>> 
>> It may be pretty uncommon, but perhaps some other examples for the type of code you're seeing would be good? I remember some code with 4 byte alignment depending upon 16-byte alignment at one point being difficult to track down due to this...
>> 
>> -eric
>> 
>> 
>> On Tue, Feb 5, 2013 at 10:53 AM, Jim Grosbach <grosbach at apple.com> wrote:
>> Thanks for clarifying.
>> 
>> With that in mind, I'm not personally opposed to demoting this to a warning anyway, but I will grant that it does somewhat weaken the case for it.
>> 
>> Eric, what do you think?
>> 
>> -Jim
>> 
>> On Feb 5, 2013, at 10:43 AM, Manman Ren <mren at apple.com> wrote:
>> 
>>> 
>>> Sorry for the confusion :]
>>> We are not performing the frontend analysis to check for whether the low bits are assumed to be zeros.
>>> I was stating that we should emit a hard error if the low bits are assumed to be zeros in the source code, and a warning if the low bits are not assumed to be zeros.
>>> But since we don't currently perform the frontend analysis, to make sure existing codes that compile with previous version can still build, we emit a warning.
>>> 
>>> Hope that clears things out.
>>> 
>>> Manman
>>> 
>>> On Feb 5, 2013, at 10:36 AM, Jim Grosbach <grosbach at apple.com> wrote:
>>> 
>>>> Hi Manman,
>>>> 
>>>>> Per discussion in rdar://13127907, we should emit a hard error only if
>>>>> people write code where the requested alignment is larger than achievable
>>>>> and assumes the low bits are zeros. A warning should be good enough when
>>>>> we are not sure if the source code assumes the low bits are zeros.
>>>> 
>>>> I'm a bit confused. This implies that we're doing the frontend analysis to check for that condition and issue a hard error for it. The below is saying that's not actually the case. Can you elaborate a bit on what exactly is happening?
>>>> 
>>>> -Jim
>>>> 
>>>> On Feb 5, 2013, at 10:18 AM, Manman Ren <mren at apple.com> wrote:
>>>> 
>>>>> 
>>>>> We currently do not analyze the source code to check the usage of the low bits.
>>>>> 
>>>>> In the backend, the alignment is already clamped to the correct value, so the backend optimizations will not treat those low bits as zero.
>>>>> 
>>>>> -Manman
>>>>> 
>>>>> On Feb 4, 2013, at 6:08 PM, Eric Christopher <echristo at gmail.com> wrote:
>>>>> 
>>>>>> Also I could be missing it but I couldn't spot the code that checks for the usage of the bits in not aligning/warning/erroring? Quick pointer?
>>>>>> 
>>>>>> Thanks!
>>>>>> 
>>>>>> -eric
>>>>>> 
>>>>>> 
>>>>>> On Mon, Feb 4, 2013 at 5:41 PM, Eric Christopher <echristo at gmail.com> wrote:
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> On Mon, Feb 4, 2013 at 5:35 PM, Manman Ren <mren at apple.com> wrote:
>>>>>> 
>>>>>> Yes, there are related discussions in r169197 and "[PATCH] Stack Alignment: clamp the alignment of stack objects in MachineFrameInfo".
>>>>>> 
>>>>>> But people can use a 32-byte alignment attribute on a machine which only supports 16-byte stack alignment.
>>>>>> If the source code further assumes the low bits are zeros, they will get wrong result.
>>>>>> But if not, a hard error is too much and it will make existing code which can compile with earlier version failed to build with this patch.
>>>>>> 
>>>>>> And to use the other side of the argument that won last time :)
>>>>>> 
>>>>>> But this means that if people aren't looking at the warning or hard erroring on warnings then we're going to emit bad code instead of making it an error.
>>>>>> 
>>>>>> -eric
>>>>>> 
>>>>>> 
>>>>> 
>>>>> _______________________________________________
>>>>> 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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130206/0d8d94ab/attachment.html>