Proposed patch (2nd revision) adds TLS Max Alignment diagnostic

[Richard Smith]

+  if (const unsigned MaxAlign = Context.getTargetInfo().getMaxTLSAlign()) {
+    for (auto *I : VD->specific_attrs<AlignedAttr>()) {
+      if (I->isAlignmentDependent())
+        return;

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?

+    }
+
+    if (VD->getTLSKind()) {
+      QualType T = VD->getType();
+      if (const ReferenceType *RT = T->getAs<ReferenceType>())
+        T = Context.getPointerType(RT->getPointeeType());

This code is redundant. The only place you use T is when you detect whether
it's dependent:

+      if (!T->isDependentType()) {

... which is unaffected by whether it's a pointer or a reference type.


+def err_tls_var_aligned_over_maximum : Error<
+  "alignment of thread-local variable %0 is greater than the maximum
supported "
+  "alignment (%1) for a thread-local variable on this target">;

Maybe include the requested alignment of the variable in this error?


+// RUN: %clang_cc1 -triple x86_64-scei-ps4 -fsyntax-only -verify %s

Please also add something like

// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only %s

... to verify that we don't emit any diagnostics for the case where we
don't have an alignment limit.

On Wed, Apr 15, 2015 at 4:04 PM, Li, Charles <
charles_li at playstation.sony.com> wrote:

>  Hi Everyone,
>
>
>
> We have updated the proposed patch which adds an error diagnostic for when
> TLS variables exceed maximum TLS alignment.
>
> This update takes care of the case where alignment is dependent. It also
> removed the unnecessary getBaseElementType() call.
>
>
>
> Sincerely,
>
> Charles Li
>
>
>
>
>
>
>
> *From:* Li, Charles
> *Sent:* Friday, April 03, 2015 3:21 PM
> *To:* 'David Majnemer'
> *Cc:* cfe-commits at cs.uiuc.edu
> *Subject:* RE: Proposed patch adds TLS Max Alignment diagnostic
>
>
>
> Hi David,
>
>
>
>
>
> Oops, we didn’t think of the case where the alignment could be a template
> parameter.
>
> I currently don’t have a clue to fix this.
>
> Any hint would be greatly welcome.
>
>
>
> On why we used BaseT instead of T.
>
> I think we originally added
>
>   QualType BaseT = Context.getBaseElementType(T);
>
> Just be to conservative as we don’t fully understand how LLVM’s type
> system when it comes to templates.
>
> I have re-ran clang replacing BaseT with T and did not notice any
> regressions.
>
> So I will take out BaseT for the next revision of the patch.
>
>
>
>
>
> Cheers,
>
> Charles Li
>
>
>
>
>
> *From:* David Majnemer [mailto:david.majnemer at gmail.com
> <david.majnemer at gmail.com>]
> *Sent:* Wednesday, April 01, 2015 7:08 PM
> *To:* Li, Charles
> *Cc:* cfe-commits at cs.uiuc.edu
> *Subject:* Re: Proposed patch adds TLS Max Alignment diagnostic
>
>
>
> Hi Charles,
>
>
>
> Your patch doesn't handle cases where the alignment is dependent,
> getDeclAlign doesn't want to be called in such cases:
>
> template <int N>
>
> struct S {
>
>   static int __thread __attribute__((aligned(N))) x;
>
> };
>
>
>
> Also, why do you use BaseT->isDependentType() instead of
> T->isDependentType()?
>
>
>
> On Wed, Apr 1, 2015 at 4:57 PM, Li, Charles <
> charles_li at playstation.sony.com> wrote:
>
> Hi Clang developers,
>
>
>
> We here at Sony PlayStation have a proposed patch which adds an error
> diagnostic for when TLS variables exceed maximum TLS alignment.
>
> Please note this patch does not affect normal maximum alignments.
>
> This TLS maximum alignment check is currently only turned on for PS4 but
> could potentially be used for other platforms.
>
>
>
> Sincerely,
>
> Charles Li
>
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150416/08394311/attachment.html>