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

Thu Feb 7 09:20:37 PST 2013

On Wed, Feb 6, 2013 at 10:39 PM, Bob Wilson <bob.wilson at apple.com> wrote:

> 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.
>

Excellent. I do remember the discussions in the past.

>  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)".
>
>
*nod*

> 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.
>
>
*nod*

> 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.
>
>
OK.

> 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.
>
>
A line number similar to how we do it for inline assembly should be
possible.

> The right way to implement this check is in the frontend.
>

I guess if we want to depend the warning on "not being able to realign the
stack to the requested alignment" we need to do it in the backend, rather
than a simple number that we could check via the front end.

>  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.
>
>
I can go with this, my only concern before was the lack of discussion.

Thanks for the explanation. All seems pretty reasonable.

-eric

> 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/20130207/8b8ddd79/attachment.html>