[PATCH] Defer codegen of dllexport/attribute(used) inline method definitions

[Hans Wennborg]

On Fri, Jun 6, 2014 at 9:56 AM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Fri, Jun 6, 2014 at 9:53 AM, Richard Smith <richard at metafoo.co.uk> wrote:
>>
>> On Thu, Jun 5, 2014 at 10:11 PM, Hans Wennborg <hans at chromium.org> wrote:
>>>
>>> ================
>>> Comment at: lib/CodeGen/ModuleBuilder.cpp:92
>>> @@ -91,3 @@
>>> -      if (!D->isDependentContext() &&
>>> -          (D->hasAttr<UsedAttr>() || D->hasAttr<ConstructorAttr>() ||
>>> -           D->hasAttr<DLLExportAttr>())) {
>>> ----------------
>>> Richard Smith wrote:
>>> > Hans Wennborg wrote:
>>> > > Richard Smith wrote:
>>> > > > An alternative fix would be to check `D->isUsed()` instead of
>>> > > > checking for `UsedAttr` here. That'd avoid the locality degradation and
>>> > > > memory allocation here.
>>> > > >
>>> > > > Or just remove this check and unconditionally call
>>> > > > `EmitTopLevelDecl`? That would better match what we do for out-of-line
>>> > > > method definitions, and would avoid duplicating this between here and
>>> > > > `ASTContext::DeclMustBeEmitted`.
>>> > > Checking D->isUsed() certainly sounds simpler :-) I'll try that out.
>>> > >
>>> > > We can't unconditionally call EmitTopLevelDecl, because it will call
>>> > > DeclMustBeEmitted, which might calculate the gva linkage, and then we fail
>>> > > this test from SemaCXX/undefined-internal.cpp :(
>>> > >
>>> > >       namespace test7 {
>>> > >         typedef struct {
>>> > >           void bar();
>>> > >           void foo() { bar(); }
>>> > >         } A;
>>> > >       }
>>> > >
>>> > > I suspect that test doesn't work well with the "used" attribute
>>> > > either.
>>> > Arrgh! That's horrible, but it does justify delaying the emission to
>>> > end of TU. The existing code is wrong for this case even without checking
>>> > 'used'. A comment here mentioning this case would be useful for future
>>> > archaeologists.
>>> The current patch delays emission to the end of the current top level
>>> declaration. Is that OK, or are you saying we need to delay all the way to
>>> the end of the TU?
>>
>>
>> End-of-top-level-decl sounds strictly better than end-of-TU =)

Great, I'll get this committed then.

> ... except that the end-of-top-level-decl callback presumably doesn't happen
> late enough (if it did, the recursive-class-walk we used to do would have
> worked too). And in particular, here:
>
> struct S {
>   struct T {
>     void foo() {}
>     void bar() { foo(); }
>   };
> };
>
> ... I think we'll have no top-level decl emitted after the method bodies are
> passed to the consumer.

Nope, this example works. HandleTopLevelDecl gets called after we've parsed S.

The class walk we used to do for inline method definitions before
r209549 was in HandleTagDeclDefinition (and it wasn't recursive).

I guess a recursive search for inline method definitions in
HandleTopLevelDecl would work, but I think the current approach of
calling HandleInlineMethodDefinition is nicer.

 - Hans