<div dir="ltr">Looks OK to me, but I'd prefer the VD->getType()->isDependentType() check to be inside hasDependentAlignment, since a variable with a dependent type is assumed to have a dependent alignment.<div><br></div><div>David: You have the most recent state on this -- can you check this is doing the right set of checks to find a dependent alignment? (Also, are we doing this check elsewhere? Should we move this check to ASTContext?)</div><div><br></div><div>One totally trivial thing to fix before commit:</div><div><br></div><div>+/// hasDependentAlignment - determines if a variable's alignment is dependent<br></div><div><br></div><div>We prefer to use \brief instead of repeating the function name in new code.</div><div><br></div><div>Thanks!<br><div class="gmail_extra"><br><div class="gmail_quote">On Thu, Apr 30, 2015 at 4:21 PM, Li, Charles <span dir="ltr"><<a href="mailto:charles_li@playstation.sony.com" target="_blank">charles_li@playstation.sony.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">





<div lang="EN-US" link="blue" vlink="purple">
<div>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Hi Everyone,<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">I am back again with the 3^rd revision of the patch which adds an error diagnostic for when TLS variables exceed maximum TLS alignment.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">This patch fixed 4 shortcomings.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p>
<p><u></u><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><span>1.<span style="font-style:normal;font-variant:normal;font-weight:normal;font-stretch:normal;font-size:7pt;line-height:normal;font-family:'Times New Roman'">     
</span></span></span><u></u><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Dependent alignment check has been spun off into a static function called hasDependentAlignment<u></u><u></u></span></p>
<p><u></u><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><span>2.<span style="font-style:normal;font-variant:normal;font-weight:normal;font-stretch:normal;font-size:7pt;line-height:normal;font-family:'Times New Roman'">     
</span></span></span><u></u><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Redundant code on checking for reference type and getting its pointee type has been removed.<u></u><u></u></span></p>
<p><u></u><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><span>3.<span style="font-style:normal;font-variant:normal;font-weight:normal;font-stretch:normal;font-size:7pt;line-height:normal;font-family:'Times New Roman'">     
</span></span></span><u></u><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Improved diagnostic to show the requested alignment of the variable<u></u><u></u></span></p>
<p><u></u><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><span>4.<span style="font-style:normal;font-variant:normal;font-weight:normal;font-stretch:normal;font-size:7pt;line-height:normal;font-family:'Times New Roman'">     
</span></span></span><u></u><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Added a Run line in the test to ensure x86_64-linux-gnu works the same as before.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Many thanks to the peer reviewers out there.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Sincerely,<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Charles Li<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"><u></u> <u></u></span></p>
<div style="border-style:solid none none;border-top-color:rgb(181,196,223);border-top-width:1pt;padding:3pt 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10pt;font-family:Tahoma,sans-serif">From:</span></b><span style="font-size:10pt;font-family:Tahoma,sans-serif"> <a href="mailto:metafoo@gmail.com" target="_blank">metafoo@gmail.com</a> [mailto:<a href="mailto:metafoo@gmail.com" target="_blank">metafoo@gmail.com</a>]
<b>On Behalf Of </b>Richard Smith<br>
<b>Sent:</b> Thursday, April 16, 2015 4:41 PM<br>
<b>To:</b> Li, Charles<br>
<b>Cc:</b> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a>; Domizioli, Dario<br>
<b>Subject:</b> Re: Proposed patch (2nd revision) adds TLS Max Alignment diagnostic<u></u><u></u></span></p>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<div>
<p class="MsoNormal">+  if (const unsigned MaxAlign = Context.getTargetInfo().getMaxTLSAlign()) {<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">+    for (auto *I : VD->specific_attrs<AlignedAttr>()) {<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">+      if (I->isAlignmentDependent())<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">+        return;<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Do not return here. You're in the middle of a function that does a bunch of other checks after this point. Perhaps factor out a static hasDependentAlignment helper function?<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">+    }<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">+<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">+    if (VD->getTLSKind()) {<u></u><u></u></p>
</div>
<div>
<div>
<p class="MsoNormal">+      QualType T = VD->getType();<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">+      if (const ReferenceType *RT = T->getAs<ReferenceType>())<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">+        T = Context.getPointerType(RT->getPointeeType());<u></u><u></u></p>
</div>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">This code is redundant. The only place you use T is when you detect whether it's dependent:<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">+      if (!T->isDependentType()) {<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">... which is unaffected by whether it's a pointer or a reference type.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<div>
<p class="MsoNormal">+def err_tls_var_aligned_over_maximum : Error<<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">+  "alignment of thread-local variable %0 is greater than the maximum supported "<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">+  "alignment (%1) for a thread-local variable on this target">;<u></u><u></u></p>
</div>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Maybe include the requested alignment of the variable in this error?<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">+// RUN: %clang_cc1 -triple x86_64-scei-ps4 -fsyntax-only -verify %s<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">Please also add something like<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only %s<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
<div>
<p class="MsoNormal">... to verify that we don't emit any diagnostics for the case where we don't have an alignment limit.<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal"><u></u> <u></u></p>
<div>
<p class="MsoNormal">On Wed, Apr 15, 2015 at 4:04 PM, Li, Charles <<a href="mailto:charles_li@playstation.sony.com" target="_blank">charles_li@playstation.sony.com</a>> wrote:<u></u><u></u></p>
<div>
<div>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Arial,sans-serif">Hi Everyone,</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Arial,sans-serif"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Arial,sans-serif">We have updated the proposed patch which adds an error diagnostic for when TLS variables exceed maximum TLS alignment.
</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Arial,sans-serif">This update takes care of the case where alignment is dependent. It also removed the unnecessary getBaseElementType()
 call.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Arial,sans-serif"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Arial,sans-serif">Sincerely,</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Arial,sans-serif">Charles Li</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"> </span><u></u><u></u></p>
<div>
<div style="border-style:solid none none;border-top-color:rgb(181,196,223);border-top-width:1pt;padding:3pt 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10pt;font-family:Tahoma,sans-serif">From:</span></b><span style="font-size:10pt;font-family:Tahoma,sans-serif"> Li, Charles
<br>
<b>Sent:</b> Friday, April 03, 2015 3:21 PM<br>
<b>To:</b> 'David Majnemer'<br>
<b>Cc:</b> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<b>Subject:</b> RE: Proposed patch adds TLS Max Alignment diagnostic</span><u></u><u></u></p>
</div>
</div>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Hi David,</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Oops, we didn’t think of the case where the alignment could be a template parameter.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">I currently don’t have a clue to fix this.
</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Any hint would be greatly welcome.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">On why we used BaseT instead of T.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">I think we originally added</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">  QualType BaseT = Context.getBaseElementType(T);</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Just be to conservative as we don’t fully understand how LLVM’s type system when it comes to templates.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">I have re-ran clang replacing BaseT with T and did not notice any regressions.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">So I will take out BaseT for the next revision of the patch.</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Cheers,</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">Charles Li</span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"> </span><u></u><u></u></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)"> </span><u></u><u></u></p>
<div style="border-style:solid none none;border-top-color:rgb(181,196,223);border-top-width:1pt;padding:3pt 0in 0in">
<p class="MsoNormal"><b><span style="font-size:10pt;font-family:Tahoma,sans-serif">From:</span></b><span style="font-size:10pt;font-family:Tahoma,sans-serif"> David Majnemer [<a href="mailto:david.majnemer@gmail.com" target="_blank">mailto:david.majnemer@gmail.com</a>]
<br>
<b>Sent:</b> Wednesday, April 01, 2015 7:08 PM<br>
<b>To:</b> Li, Charles<br>
<b>Cc:</b> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<b>Subject:</b> Re: Proposed patch adds TLS Max Alignment diagnostic</span><u></u><u></u></p>
</div>
<p class="MsoNormal"> <u></u><u></u></p>
<div>
<p class="MsoNormal">Hi Charles,<u></u><u></u></p>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">Your patch doesn't handle cases where the alignment is dependent, getDeclAlign doesn't want to be called in such cases:<u></u><u></u></p>
</div>
<div>
<div>
<p class="MsoNormal">template <int N><u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">struct S {<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">  static int __thread __attribute__((aligned(N))) x;<u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">};<u></u><u></u></p>
</div>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
<div>
<p class="MsoNormal">Also, why do you use BaseT->isDependentType() instead of T->isDependentType()?<u></u><u></u></p>
</div>
</div>
<div>
<p class="MsoNormal"> <u></u><u></u></p>
<div>
<p class="MsoNormal">On Wed, Apr 1, 2015 at 4:57 PM, Li, Charles <<a href="mailto:charles_li@playstation.sony.com" target="_blank">charles_li@playstation.sony.com</a>> wrote:<u></u><u></u></p>
<div>
<div>
<p class="MsoNormal">Hi Clang developers,<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">We here at Sony PlayStation have a proposed patch which adds an error diagnostic for when TLS variables exceed maximum TLS alignment.
<u></u><u></u></p>
<p class="MsoNormal">Please note this patch does not affect normal maximum alignments.
<u></u><u></u></p>
<p class="MsoNormal">This TLS maximum alignment check is currently only turned on for PS4 but could potentially be used for other platforms.<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
<p class="MsoNormal">Sincerely,<u></u><u></u></p>
<p class="MsoNormal">Charles Li<u></u><u></u></p>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
</div>
<p class="MsoNormal" style="margin-bottom:12pt"><br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><u></u><u></u></p>
</div>
<p class="MsoNormal"> <u></u><u></u></p>
</div>
</div>
</div>
<p class="MsoNormal" style="margin-bottom:12pt"><br>
_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><u></u><u></u></p>
</div>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div>
</div>
</div>

<br>_______________________________________________<br>
cfe-commits mailing list<br>
<a href="mailto:cfe-commits@cs.uiuc.edu">cfe-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a><br>
<br></blockquote></div><br></div></div></div>