Proposed patch (4th revision) adds TLS Max Alignment diagnostic

Wed May 27 15:24:10 PDT 2015

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> 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; '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 <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; 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> 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] *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> 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
>
>
>
>
> _______________________________________________
> 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/20150527/dd519541/attachment.html>