<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; ">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:<div><br></div><div>1. Confusion about the alignment being specified in bytes, not bits. E.G.: Someone wanted a 4-byte aligned buffer and wrote "aligned(32)".</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div><div><div>On Feb 5, 2013, at 5:04 PM, Manman Ren <<a href="mailto:mren@apple.com">mren@apple.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><meta http-equiv="Content-Type" content="text/html charset=iso-8859-1"><div style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><br><div><div>On Feb 5, 2013, at 11:03 AM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr">Well, I think we've gone from a compile time error for code like this:<div><br></div><div style="">void foo () {</div><div style=""> my_32byte_align_requested_struct a;</div><div style=""><br></div><div style=""> <insert inline asm that depends upon 32-byte alignment></div>
<div style=""><br></div><div style="">}</div><div style=""><br></div><div style="">to a run time crash.</div></div></blockquote><div><br></div>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.</div><div>If the source code does not depend on the 32-byte alignment, we are going from a compile time error to a warning.</div><div><br></div><div>Before r<span style="font-size: 12px;">172027, it is a run time crash without any warning.</span></div><div><span style="font-size: 12px;"><br></span></div><div><span style="font-size: 12px;">Let me know what you think :]</span></div><div><span style="font-size: 12px;"><br></span></div><div><span style="font-size: 12px;">Thanks,</span></div><div><span style="font-size: 12px;">Manman</span></div><div><span style="font-size: 12px;"> </span><br><blockquote type="cite"><div dir="ltr"><div style=""><br></div><div style="">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...</div>
<div style=""><br></div><div style="">-eric</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Feb 5, 2013 at 10:53 AM, Jim Grosbach <span dir="ltr"><<a href="mailto:grosbach@apple.com" target="_blank">grosbach@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin: 0px 0px 0px 0.8ex; border-left-width: 1px; border-left-color: rgb(204, 204, 204); border-left-style: solid; padding-left: 1ex; position: static; z-index: auto;"><div style="word-wrap:break-word">Thanks for clarifying.<div><br></div><div>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.</div>
<div><br></div><div>Eric, what do you think?</div><span class="HOEnZb"><font color="#888888"><div><br></div><div>-Jim</div></font></span><div><div class="h5"><br><div><div>On Feb 5, 2013, at 10:43 AM, Manman Ren <<a href="mailto:mren@apple.com" target="_blank">mren@apple.com</a>> wrote:</div>
<br><blockquote type="cite"><div style="word-wrap:break-word"><div><br></div>Sorry for the confusion :]<div>We are not performing the frontend analysis to check for whether the low bits are assumed to be zeros.</div><div>
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.</div><div>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.</div>
<div><br></div><div>Hope that clears things out.</div><div><br></div><div>Manman</div><div><br><div><div>On Feb 5, 2013, at 10:36 AM, Jim Grosbach <<a href="mailto:grosbach@apple.com" target="_blank">grosbach@apple.com</a>> wrote:</div>
<br><blockquote type="cite"><div style="word-wrap:break-word">Hi Manman,<div><br></div><div><blockquote type="cite">Per discussion in <a>rdar://13127907</a>, we should emit a hard error only if<br>people write code where the requested alignment is larger than achievable<br>
and assumes the low bits are zeros. A warning should be good enough when<br>we are not sure if the source code assumes the low bits are zeros.</blockquote><div><br></div><div>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?</div>
</div></div></blockquote><blockquote type="cite"><div style="word-wrap:break-word"><div><br></div><div>-Jim</div><div><br><div><div>On Feb 5, 2013, at 10:18 AM, Manman Ren <<a href="mailto:mren@apple.com" target="_blank">mren@apple.com</a>> wrote:</div>
<br><blockquote type="cite"><div style="word-wrap:break-word"><div><br></div>We currently do not analyze the source code to check the usage of the low bits.<div><br><div>In the backend, the alignment is already clamped to the correct value, so the backend optimizations will not treat those low bits as zero.</div>
<div><br></div><div>-Manman</div><div><br></div><div><div><div>On Feb 4, 2013, at 6:08 PM, Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>> wrote:</div><br><blockquote type="cite">
<div dir="ltr">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?<div><br></div><div>Thanks!</div><div><br></div><div>
-eric</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Mon, Feb 4, 2013 at 5:41 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote"><div>On Mon, Feb 4, 2013 at 5:35 PM, Manman Ren <span dir="ltr"><<a href="mailto:mren@apple.com" target="_blank">mren@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><br></div>Yes, there are related discussions in r169197 and "[PATCH] Stack Alignment: clamp the alignment of stack objects in MachineFrameInfo".<div>
<br></div><div>But people can use a 32-byte alignment attribute on a machine which only supports 16-byte stack alignment.</div><div>If the source code further assumes the low bits are zeros, they will get wrong result.</div>
<div>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.</div></div></blockquote><div><br></div></div><div>And to use the other side of the argument that won last time :)</div>
<div><br></div><div>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.</div><span><font color="#888888"><div>
<br></div><div>
-eric</div></font></span></div><br></div></div>
</blockquote></div><br></div>
</blockquote></div><br></div></div></div>
_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu" target="_blank">llvm-commits@cs.uiuc.edu</a><br><a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
</blockquote></div><br></div></div></blockquote></div><br></div></div></blockquote></div><br></div></div></div></blockquote></div><br></div>
</blockquote></div><br></div>_______________________________________________<br>llvm-commits mailing list<br><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits<br></blockquote></div><br></div></body></html>