Proposed patch (5th revision) adds TLS Max Alignment diagnostic
Robinson, Paul
Paul_Robinson at playstation.sony.com
Tue Jul 14 13:53:29 PDT 2015
I committed this for Charles, r242198.
--paulr
From: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-bounces at cs.uiuc.edu] On Behalf Of Li, Charles
Sent: Friday, July 10, 2015 1:25 PM
To: David Majnemer
Cc: cfe-commits at cs.uiuc.edu
Subject: RE: Proposed patch (5th revision) adds TLS Max Alignment diagnostic
Hi David,
Thank you for reviewing my patch.
I have incorporated all of your advice and synced the patch up to the latest Trunk.
I don’t have commit privilege to Trunk, would it be possible for you to commit it on my behalf?
Alternatively, if you are busy, I can ask someone here at Sony to commit it for me.
Yours Sincerely,
Charles Li
From: David Majnemer [mailto:david.majnemer at gmail.com]
Sent: Friday, July 10, 2015 12:27 AM
To: Li, Charles
Subject: Re: Proposed patch (5th revision) adds TLS Max Alignment diagnostic
+/// \brief Determines if a variable's alignment is dependent.
+static bool hasDependentAlignment(VarDecl *VD) {
+ if (VD->getType()->isDependentType())
+ return true;
+ for (auto *I : VD->specific_attrs<AlignedAttr>()) {
+ if (I->isAlignmentDependent())
+ return true;
+ }
+ return false;
The for-loop's curly braces are superfluous.
+ if (const unsigned MaxAlign = Context.getTargetInfo().getMaxTLSAlign()) {
Drop the const.
+ if ( VarDecl *VD = dyn_cast<VarDecl>(D) ) {
+ if ( VD->getTLSKind() ) {
Don't have extra whitespace inside the if-statement's parenthesis.
+ const CharUnits MaxAlignChars = Context.toCharUnitsFromBits(MaxAlign);
Drop the const here.
LGTM otherwise.
On Thu, Jul 9, 2015 at 2:14 PM, Li, Charles <charles_li at playstation.sony.com<mailto:charles_li at playstation.sony.com>> wrote:
Hello David,
Sorry to bother you again.
I would really appreciate a code review as I hope to have this patch make it into 3.7 release.
Yours Sincerely,
Charles Li
From: Li, Charles
Sent: Tuesday, June 30, 2015 10:04 AM
To: 'David Majnemer'; 'cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>'
Cc: 'Richard Smith'
Subject: FW: Proposed patch (4th revision) adds TLS Max Alignment diagnostic
Pinging David,
Hi David,
I would really appreciate your opinion on my latest patch.
Thanks a bunch.
Sincerely,
Charles Li
From: Li, Charles
Sent: Wednesday, June 17, 2015 7:08 PM
To: 'David Majnemer'; cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>
Cc: Richard Smith
Subject: RE: Proposed patch (4th revision) adds TLS Max Alignment diagnostic
Hi David,
I have removed the run line for x86_64-linux-gnu.
In addition, I have also added TLS Max Align instantiation check for the case where the alignment value is a template parameter.
/*****************************************/
template <int N>
struct S {
static int __thread __attribute__((aligned(N))) x;
};
S<64> s_instance;
/*****************************************/
This case used to generate no diagnostic, now it will generate the following diagnostics.
/****************************************************************************************************/
inst.cpp:3:51: error: alignment (64) of thread-local variable 'x' is greater than the maximum supported alignment (32) for a
thread-local variable on this target
static int __thread __attribute__((aligned(N))) x;
^
inst.cpp:6:7: note: in instantiation of template class 'S<64>' requested here
S<64> s_instance;
^
/****************************************************************************************************/
Please let me know what you think.
Sincerely,
Charles Li
From: David Majnemer [mailto:david.majnemer at gmail.com]
Sent: Wednesday, May 27, 2015 3:24 PM
To: Li, Charles
Cc: cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>; Domizioli, Dario; Richard Smith
Subject: Re: Proposed patch (4th revision) adds TLS Max Alignment diagnostic
One of your run lines doesn't do anything:
+// RUN: %clang_cc1 -triple x86_64-linux-gnu -fsyntax-only %s
Please remove it.
On Fri, May 1, 2015 at 12:10 PM, Li, Charles <charles_li at playstation.sony.com<mailto:charles_li at playstation.sony.com>> wrote:
Hi David,
I have revised the TLS Max Align patch taken into account of all the reviews from you and Richard.
I would very much appreciate another code review.
Also Richard has the following question for you from the previous email (review for the 3rd revision of the patch). I would also appreciate your input.
> 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?)
Thank you,
Charles Li
From: Li, Charles
Sent: Thursday, April 30, 2015 6:48 PM
To: cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>; 'Richard Smith'; David Majnemer
Cc: Domizioli, Dario
Subject: RE: Proposed patch (4th revision) adds TLS Max Alignment diagnostic
Hi Richard,
Thank you very much for pointing out all the intricacies I was not aware of.
Both points have been addressed.
1. Dependent type check has been moved into the function hasDependentAlignment
2. The previous comment
/// hasDependentAlignment - determines if a variable's alignment is dependent
has been changed to
/// \brief Determines if a variable's alignment is dependent.
Please let me know if there are any more bugs. I very much appreciate it.
Sincerely,
Charles Li
From: metafoo at gmail.com<mailto:metafoo at gmail.com> [mailto:metafoo at gmail.com] On Behalf Of Richard Smith
Sent: Thursday, April 30, 2015 4:43 PM
To: Li, Charles; David Majnemer
Cc: cfe-commits at cs.uiuc.edu<mailto:cfe-commits at cs.uiuc.edu>; Domizioli, Dario
Subject: Re: Proposed patch (3rd revision) adds TLS Max Alignment diagnostic
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.
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?)
One totally trivial thing to fix before commit:
+/// hasDependentAlignment - determines if a variable's alignment is dependent
We prefer to use \brief instead of repeating the function name in new code.
Thanks!
On Thu, Apr 30, 2015 at 4:21 PM, Li, Charles <charles_li at playstation.sony.com<mailto:charles_li at playstation.sony.com>> wrote:
Hi Everyone,
I am back again with the 3rd revision of the patch which adds an error diagnostic for when TLS variables exceed maximum TLS alignment.
This patch fixed 4 shortcomings.
1. Dependent alignment check has been spun off into a static function called hasDependentAlignment
2. Redundant code on checking for reference type and getting its pointee type has been removed.
3. Improved diagnostic to show the requested alignment of the variable
4. Added a Run line in the test to ensure x86_64-linux-gnu works the same as before.
Many thanks to the peer reviewers out there.
Sincerely,
Charles Li
From: metafoo at gmail.com<mailto:metafoo at gmail.com> [mailto: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<mailto: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
_______________________________________________
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/20150714/50316db9/attachment.html>
More information about the cfe-commits
mailing list