r205810 - [MS-ABI] Add support for #pragma section and related pragmas

[John McCall]

> On Mar 4, 2015, at 3:30 PM, Reid Kleckner <rnk at google.com> wrote:
> > +  if (D.isFunctionDefinition() && CodeSegStack.CurrentValue &&
> 
> D.isFunctionDefinition() is cheap, but CodeSegStack.CurrentValue is just as
> cheap and much more likely to short-circuit; you should check it first.
> 
> Sure
> > +  if (var->isThisDeclarationADefinition() &&
> > +      ActiveTemplateInstantiations.empty()) {
> > +    PragmaStack<StringLiteral *> *Stack = nullptr;
> > +    int SectionFlags = PSF_Implicit | PSF_Read;
> > +    if (var->getType().isConstQualified())
> > +      Stack = &ConstSegStack;
> > +    else if (!var->getInit()) {
> > +      Stack = &BSSSegStack;
> > +      SectionFlags |= PSF_Write;
> > +    } else {
> > +      Stack = &DataSegStack;
> > +      SectionFlags |= PSF_Write;
> > +    }
> > +    if (!var->hasAttr<SectionAttr>() && Stack->CurrentValue)
> 
> You should check Stack->CurrentValue first, it’s both cheaper and more likely to short-circuit.
> 
> Sure
>  
> In fact, you should consider just keeping a flag on Sema about whether *any* data section pragmas are active, since I assume that none are for the vast majority of time, and it would be good to avoid this entire block in that case.  If you don’t do that, you should at least guard the block on getLangOpts().MicrosoftExt.
> 
> I think we want to come in here in !MicrosoftExt mode to do UnifySection. That helps us diagnose this:
> __attribute__((section(".mydata"))) extern const int x = 42;
> __attribute__((section(".mydata"))) int y = 42;
> t.cpp:2:41: error: 'y' causes a section type conflict with ‘x'

Okay, but you could still trigger that check when applying an explicit section attribute instead of checking it for every variable.  I know it would require a little bit of logic duplication for computing the flags, but I think it would be worth it.

Also, the code seems to be doing some redundant lookups of the section attribute.

Is this section-attributes conflict thing actually a problem for ELF, or is this COFF-specific?  On Mach-O, this is already encoded in the section name, so it’s not really possible to have a conflict.

Anyway, thanks for fixing this.

John.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20150304/a5372143/attachment.html>