<div dir="ltr"><div>+  if (const unsigned MaxAlign = Context.getTargetInfo().getMaxTLSAlign()) {</div><div>+    for (auto *I : VD->specific_attrs<AlignedAttr>()) {</div><div>+      if (I->isAlignmentDependent())</div><div>+        return;</div><div><br></div><div>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?</div><div><br></div><div>+    }</div><div>+</div><div>+    if (VD->getTLSKind()) {</div><div><div>+      QualType T = VD->getType();</div><div>+      if (const ReferenceType *RT = T->getAs<ReferenceType>())</div><div>+        T = Context.getPointerType(RT->getPointeeType());</div></div><div><br></div><div>This code is redundant. The only place you use T is when you detect whether it's dependent:</div><div><br></div><div>+      if (!T->isDependentType()) {<br></div><div><br></div><div>... which is unaffected by whether it's a pointer or a reference type.</div><div><br></div><div><br></div><div><div>+def err_tls_var_aligned_over_maximum : Error<</div><div>+  "alignment of thread-local variable %0 is greater than the maximum supported "</div><div>+  "alignment (%1) for a thread-local variable on this target">;</div></div><div><br></div><div>Maybe include the requested alignment of the variable in this error?</div><div><br></div><div><br></div><div>+// RUN: %clang_cc1 -triple x86_64-scei-ps4 -fsyntax-only -verify %s<br></div><div><br></div><div>Please also add something like</div><div><br></div><div>// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only %s</div><div><br></div><div>... to verify that we don't emit any diagnostics for the case where we don't have an alignment limit.</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Apr 15, 2015 at 4:04 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:Arial,sans-serif">Hi Everyone,<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Arial,sans-serif"><u></u> <u></u></span></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.
<u></u><u></u></span></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.<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Arial,sans-serif"><u></u> <u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Arial,sans-serif">Sincerely,<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Arial,sans-serif">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>
<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>
<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<u></u><u></u></span></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,<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)">Oops, we didn’t think of the case where the alignment could be a template parameter.<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 currently don’t have a clue to fix this.
<u></u><u></u></span></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.<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)">On why we used BaseT instead of T.<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 think we originally added<u></u><u></u></span></p>
<p class="MsoNormal"><span style="font-size:11pt;font-family:Calibri,sans-serif;color:rgb(31,73,125)">  QualType BaseT = Context.getBaseElementType(T);<u></u><u></u></span></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.<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 have re-ran clang replacing BaseT with T and did not notice any regressions.<u></u><u></u></span></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.<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)">Cheers,<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"> 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<u></u><u></u></span></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>

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