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

[Eric Christopher]

>
> A line number similar to how we do it for inline assembly should be
> possible.
>
>
> We could get close but the source location info available in the backend
> isn't as good as what we can get out of the frontend.
>


> I think we can teach the front-end to know when the backend can support
> the realignment.  That's my plan, anyway.
>
>  Cool deal. Best of luck :)

Maybe time to revisit some of the "duplicate some code between front end
and back end" discussions?

-eric


>
>
>>  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
>
>
> OK, good.  I've gone ahead and reverted 172027 and 174336 in 174741.  I'm
> also going to revert that broken emitWarning method that I added earlier
> before anyone else starts using it.
>
>
>
>
>> 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/20130208/fca01347/attachment.html>