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

[John McCall]

> On Apr 8, 2014, at 3:30 PM, Warren Hunt <whunt at google.com> wrote:
> Author: whunt
> Date: Tue Apr  8 17:30:47 2014
> New Revision: 205810
> 
> URL: http://llvm.org/viewvc/llvm-project?rev=205810&view=rev
> Log:
> [MS-ABI] Add support for #pragma section and related pragmas
> This patch adds support for the msvc pragmas section, bss_seg, code_seg, 
> const_seg and data_seg as well as support for __declspec(allocate()).
> 
> Additionally it corrects semantics and adds diagnostics for 
> __attribute__((section())) and the interaction between the attribute 
> and the msvc pragmas and declspec.  In general conflicts should now be 
> well diganosed within and among these features.
> 
> In supporting the pragmas new machinery for uniform lexing for 
> msvc pragmas was introduced.  The new machinery always lexes the 
> entire pragma and stores it on an annotation token.  The parser 
> is responsible for parsing the pragma when the handling the 
> annotation token.
> 
> There is a known outstanding bug in this implementation in C mode.  
> Because these attributes and pragmas apply _only_ to definitions, we 
> process them at the time we detect a definition.  Due to tentative 
> definitions in C, we end up processing the definition late.  This means 
> that in C mode, everything that ends up in a BSS section will end up in 
> the _last_ BSS section rather than the one that was live at the time of 
> tentative definition, even if that turns out to be the point of actual 
> definition.  This issue is not known to impact anything as of yet 
> because we are not aware of a clear use or use case for #pragma bss_seg 
> but should be fixed at some point.

Sorry for the late review. :)

> +  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.

> +      !NewFD->hasAttr<SectionAttr>()) {
> +    NewFD->addAttr(
> +        SectionAttr::CreateImplicit(Context, SectionAttr::Declspec_allocate,
> +                                    CodeSegStack.CurrentValue->getString(),
> +                                    CodeSegStack.CurrentPragmaLocation));
> +    if (UnifySection(CodeSegStack.CurrentValue->getString(),
> +                     PSF_Implicit | PSF_Execute | PSF_Read, NewFD))
> +      NewFD->dropAttr<SectionAttr>();
> +  }
> +
>   // Handle attributes.
>   ProcessDeclAttributes(S, NewFD, D);
> 
> @@ -8929,6 +8940,29 @@ void Sema::CheckCompleteVariableDeclarat
>       Diag(var->getLocation(), diag::note_use_thread_local);
>   }
> 
> +  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.

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.

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.

> +      var->addAttr(
> +          SectionAttr::CreateImplicit(Context, SectionAttr::Declspec_allocate,
> +                                      Stack->CurrentValue->getString(),
> +                                      Stack->CurrentPragmaLocation));
> +    if (const SectionAttr *SA = var->getAttr<SectionAttr>())
> +      if (UnifySection(SA->getName(), SectionFlags, var))
> +        var->dropAttr<SectionAttr>();
> +  }
> +

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.

John.