On 09.06.2014 09:29, Christian Kamm wrote:
> My runtime/std/stdio.o also only has a
> U _D6object15__T8capacityTaZ8capacityFNaNbNdAaZm
> so I think I can reproduce it locally.

The symbol isn't emitted because its instantiatingModule is std.bitmanip - which is not a root module - and thus the function is ignored by DtoDefineFunction.

I think the comment in there (functions.cpp:922) is wrong. The frontend seems to try hard to make sure instantiatingModule is a non-root module if possible. That should mean LDC is not emitting templates that have a non-root module instantiating them somewhere.

The idea probably is that you shouldn't need to emit functions again if they were already emitted into a library you import and link. (if that's desired, the correct fix is probably to require -lcurl when linking phobos...)

I wonder if you can break that behavior with a cycle of imports-that-instantiate. But I couldn't make a failing test case.

Cheers,
Christian

On 9 Jun 2014, at 10:44, Christian Kamm via digitalmars-d-ldc wrote:
> I think the comment in there (functions.cpp:922) is wrong. The frontend
> seems to try hard to make sure instantiatingModule is a non-root module
> if possible. That should mean LDC is not emitting templates that have a
> non-root module instantiating them somewhere.

This logic was adapted from what I gathered from discussions when 2.064 (I think) came out. If you look at FuncDeclaration::toObjFile in DMD 2.064.2, you'll see that it uses the same logic to determine whether to emit a certain symbol.

In more recent versions, Kenji has moved the check into the frontend at our (GDC/LDC) request, but it is still fundamentally the same (FuncDeclaration::needsCodegen, https://github.com/D-Programming-Language/dmd/pull/3107).

> The idea probably is that you shouldn't need to emit functions again if
> they were already emitted into a library you import and link. (if that's
> desired, the correct fix is probably to require -lcurl when linking
> phobos...)

The idea is instead that functions that are already part of an *object file* you need to link anyway should not be emitted again. This is a sound design, as long as you only omit template instances that you know are already required by somebody else in your dependency graph (ignoring cycles for the moment).

Now, obviously std.net.curl isn't in the import graph of std.stdio. What seems to happen here is that std.net.curl only contains the symbol by accident, even though we thought it was going to be provided by somebody else. And as we don't build with symbol-per-section and --gc-sections yet, this of course causes us to also pull in the libcurl dependencies. Thinking about this a bit, it seems very plausible that the compiler actually works as intended here.

This suggests that a possible fix would be to split off everything that depends on curl into a separate static library, as this would guarantee that the linker looks up the object files from the non-curl modules first (but then, of course, we'd either have to specify libphobos-ldc twice, or use the GNU ld grouping options, to get std.net.curl to link with its Phobos dependencies).

Best,
David

On 09.06.2014 13:09, David Nadlinger via digitalmars-d-ldc wrote:
> On 9 Jun 2014, at 10:44, Christian Kamm via digitalmars-d-ldc wrote:
>> I think the comment in there (functions.cpp:922) is wrong. The frontend seems to try hard to make sure instantiatingModule is a non-root module if possible. That should mean LDC is not emitting templates that have a non-root module instantiating them somewhere.
> 
> This logic was adapted from what I gathered from discussions when 2.064 (I think) came out. If you look at FuncDeclaration::toObjFile in DMD 2.064.2, you'll see that it uses the same logic to determine whether to emit a certain symbol.

Yes, I saw. I just think the comment is misleading: it says
"Skip generating code if this part of a TemplateInstance that is
instantiated only by non-root modules"
but actually it seems to skip instances that have any non-root module
instantiating them. I'll make a pull request to fix it.


>> The idea probably is that you shouldn't need to emit functions again if they were already emitted into a library you import and link. (if that's desired, the correct fix is probably to require -lcurl when linking phobos...)
> 
> The idea is instead that functions that are already part of an *object file* you need to link anyway should not be emitted again. This is a sound design, as long as you only omit template instances that you know are already required by somebody else in your dependency graph (ignoring cycles for the moment).

Okay. Aside: how does it deal with cycles? Wouldn't no instance be emitted if two modules both instantiate the same function and include each other? (in practice both were emitted for me)


> Thinking about this a bit, it seems very plausible that the compiler actually works as intended here.

Agreed.

In dmd's libphobos2, array_10f_5e7.o and array_187_86f.o use the symbol while only object_5_50d.o defines it. Why doesn't dmd's stdio use it?


Regards,
Christian

Hi Christian,

On 9 Jun 2014, at 18:28, Christian Kamm via digitalmars-d-ldc wrote:
> I'll make a pull request to fix it.

Yes, that would be great.

> Okay. Aside: how does it deal with cycles? Wouldn't no instance be
> emitted if two modules both instantiate the same function and include
> each other? (in practice both were emitted for me)

If ti->instantiatingModule (the module you would like to pull the symbol in from) itself also imports at least one of the root modules, then importsRoot will be true, and the symbol will still be defined. Exactly the same situation will occur (with m/mi swapped) when building what currently is ti->instantiatingModule, so you end up emitting that template into both modules, as you observed.

On a somewhat unrelated note, the use of "insearch" in that piece of code is a beautiful example of DMD's … uhm … pasta-inspired design.

Best,
David

On 10.06.2014 14:09, David Nadlinger via digitalmars-d-ldc wrote:
>> Okay. Aside: how does it deal with cycles? Wouldn't no instance be emitted if two modules both instantiate the same function and include each other? (in practice both were emitted for me)
> 
> If ti->instantiatingModule (the module you would like to pull the symbol in from) itself also imports at least one of the root modules, then importsRoot will be true, and the symbol will still be defined. Exactly the same situation will occur (with m/mi swapped) when building what currently is ti->instantiatingModule, so you end up emitting that template into both modules, as you observed.

Oh, right! Thanks for clearing that up for me.

Cheers,
Christian

On Monday, 9 June 2014 at 16:28:11 UTC, Christian Kamm wrote:
> On 09.06.2014 13:09, David Nadlinger via digitalmars-d-ldc wrote:
> Yes, I saw. I just think the comment is misleading: it says
> "Skip generating code if this part of a TemplateInstance that is
> instantiated only by non-root modules"
> but actually it seems to skip instances that have any non-root module
> instantiating them. I'll make a pull request to fix it.

That's a nice summary what the code does. A pull request would be great!

Regards,
Kai

> Thinking about this a bit, it seems very plausible that the compiler actually works as intended here.

For what it's worth, compiling stdio.d with dmd 2.064 also does not emit object.capacity(). So if dmd's phobos was built the same way as ldc's, I expect it'd have the same issue.

It seems like we could imitate what dmd -lib does or use --gc-sections like David suggested to fix this for real.

Could we, as a workaround, reorder the object files in the phobos
library to make std.net.curl come last? Does the static linker work that
way? It's annoying that this blocks a new ldc version from being released.

Regards,
Christian

On 14.06.2014 14:18, Christian Kamm wrote:
>> Thinking about this a bit, it seems very plausible that the compiler actually works as intended here.
> 
> For what it's worth, compiling stdio.d with dmd 2.064 also does not emit object.capacity(). So if dmd's phobos was built the same way as ldc's, I expect it'd have the same issue.
> 
> It seems like we could imitate what dmd -lib does or use --gc-sections like David suggested to fix this for real.
> 
> Could we, as a workaround, reorder the object files in the phobos
> library to make std.net.curl come last? Does the static linker work that
> way? It's annoying that this blocks a new ldc version from being released.

Maybe removing the call to ranlib or using ranlib -D could help? The order in the archive looks fine (string.o before curl.o).

On Sat, Jun 14, 2014 at 2:18 PM, Christian Kamm via digitalmars-d-ldc <digitalmars-d-ldc@puremagic.com> wrote:
> It's annoying that this blocks a new ldc version from being released.

At this point, I think _any_ fix for the issue would be fine. We really need to get merge-2.064 and merge-2.065 out there.

Unfortunately, I still can't reproduce the issue. The Travis CI docs say that they run Ubuntu 12.04 LTS, but I couldn't get the linker error to appear on a EC2 instance I set up from the Canonical AMI.

Best,
David

On 14.06.2014 20:28, David Nadlinger via digitalmars-d-ldc wrote:
> Unfortunately, I still can't reproduce the issue. The Travis CI docs say that they run Ubuntu 12.04 LTS, but I couldn't get the linker error to appear on a EC2 instance I set up from the Canonical AMI.

Is it possible to log into the Travis instances? If it is, looking at the output and playing around with the static linker flags could help finding a workaround.

I'd be interested in nm -s on that libphobos2.a as well as the order of files in the archive and the symbols in the object files.

I don't think ranlib -D would change anything - it seems to only force some meta information to 0, not change the lookup order.

Cheers,
Christian