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 < [email protected]> 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:* [email protected] [mailto:[email protected]] *On Behalf Of *Richard > Smith > *Sent:* Thursday, April 16, 2015 4:41 PM > *To:* Li, Charles > *Cc:* [email protected]; 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 < > [email protected]> 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:* [email protected] > *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:[email protected] > <[email protected]>] > *Sent:* Wednesday, April 01, 2015 7:08 PM > *To:* Li, Charles > *Cc:* [email protected] > *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 < > [email protected]> 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 > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > > > > _______________________________________________ > cfe-commits mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits > >
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
