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

[Li, Charles]

Hi Richard,

Thank you for this insightful review. I am thoroughly schooled by the great ways of LLVM.
We will go back to the drawing board and investigate into each point you have raised.

Sincerely,
Charles Li

From: metafoo at gmail.com [mailto:metafoo at gmail.com] On Behalf Of Richard Smith
Sent: Thursday, April 16, 2015 4:41 PM
To: Li, Charles
Cc: cfe-commits at cs.uiuc.edu; Domizioli, Dario
Subject: Re: Proposed patch (2nd revision) adds TLS Max Alignment diagnostic

+  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<mailto: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<mailto: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]
Sent: Wednesday, April 01, 2015 7:08 PM
To: Li, Charles
Cc: cfe-commits at cs.uiuc.edu<mailto: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<mailto: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<mailto: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<mailto: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/20150417/88d96afb/attachment.html>