<div dir="ltr">Should be addressed in r231317.</div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Mar 4, 2015 at 3:30 PM, Reid Kleckner <span dir="ltr"><<a href="mailto:rnk@google.com" target="_blank">rnk@google.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span>> +  if (D.isFunctionDefinition() && CodeSegStack.CurrentValue &&<br>
<br>
</span>D.isFunctionDefinition() is cheap, but CodeSegStack.CurrentValue is just as<br>
cheap and much more likely to short-circuit; you should check it first.</blockquote><div><br></div></span><div>Sure</div><span class=""><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div><div>
> +  if (var->isThisDeclarationADefinition() &&<br>
> +      ActiveTemplateInstantiations.empty()) {<br>
> +    PragmaStack<StringLiteral *> *Stack = nullptr;<br>
> +    int SectionFlags = PSF_Implicit | PSF_Read;<br>
> +    if (var->getType().isConstQualified())<br>
> +      Stack = &ConstSegStack;<br>
> +    else if (!var->getInit()) {<br>
> +      Stack = &BSSSegStack;<br>
> +      SectionFlags |= PSF_Write;<br>
> +    } else {<br>
> +      Stack = &DataSegStack;<br>
> +      SectionFlags |= PSF_Write;<br>
> +    }<br>
> +    if (!var->hasAttr<SectionAttr>() && Stack->CurrentValue)<br>
<br>
</div></div>You should check Stack->CurrentValue first, it’s both cheaper and more likely to short-circuit.<br></blockquote><div><br></div></span><div>Sure</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
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.<br></blockquote><div><br></div></span><div>I think we want to come in here in !MicrosoftExt mode to do UnifySection. That helps us diagnose this:</div><div><div>__attribute__((section(".mydata"))) extern const int x = 42;</div><div>__attribute__((section(".mydata"))) int y = 42;</div></div><div><div>t.cpp:2:41: error: 'y' causes a section type conflict with 'x'</div></div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
You should also not apply a section attribute to a variable of automatic storage duration, and you should check with MSVC about whether they apply to local statics and static member variables, if you haven’t already.</blockquote><div><br></div></span><div>MSVC applies these pragmas to static locals and static data members, but not locals obviously. I can short-circuit out of the stack computation for local variables.</div><span class=""><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><span><br>
> +      var->addAttr(<br>
> +          SectionAttr::CreateImplicit(Context, SectionAttr::Declspec_allocate,<br>
> +                                      Stack->CurrentValue->getString(),<br>
> +                                      Stack->CurrentPragmaLocation));<br>
> +    if (const SectionAttr *SA = var->getAttr<SectionAttr>())<br>
> +      if (UnifySection(SA->getName(), SectionFlags, var))<br>
> +        var->dropAttr<SectionAttr>();<br>
> +  }<br>
> +<br>
<br>
</span>Please ask the target to validate the section name with TargetInfo::isValidSectionSpecifier so that a malformed section doesn't simply crash the backend.  It’s probably best to do that at the point of the pragma.<br></blockquote><div><br></div></span><div>OK </div></div></div></div>
</blockquote></div><br></div>