Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix Issue 19570 - pragma(inline) is emitting symbols #9235

Closed
wants to merge 1 commit into from

Conversation

thewilsonator
Copy link
Contributor

@dlang-bot
Copy link
Contributor

Thanks for your pull request and interest in making D better, @thewilsonator! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
19570 critical pragma(inline) is emitting symbols

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

To target stable perform these two steps:

  1. Rebase your branch to upstream/stable:
git rebase --onto upstream/stable upstream/master
  1. Change the base branch of your PR to stable

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "master + dmd#9235"

@TurkeyMan
Copy link
Contributor

Interesting that this causes link errors... as if things are depending on external inline functions.
Or did those calls just decide not to inline for some reason? Or perhaps there was something preventing them from inlining?

I think as a first step to resist breakage, we should probably emit the functions as usual, but only if the function is actually referenced...
I wonder if we can mark that a FunctionDeclaration is actually involved in a call?
Ie, add a wasCalled bool, and when functions are called, assign true...?

@thewilsonator
Copy link
Contributor Author

Interesting that this causes link errors...

Bummer, although I can't say I'm surprised.

@TurkeyMan
Copy link
Contributor

I feel like every error emit here is a bug...

@WalterBright
Copy link
Member

I question the validity of this as a bug, let alone a critical bug. I posted the reasons to bugzilla. Please continue discussion about that there, the discussion here should be about problems with the PR itself, not the validity of the issue.

Copy link
Member

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the reported issue is a bug.

Also:

There should only be a symbol in the object file in the case it failed to inline, and only if it's actually called.

What about separate compilation ? If it's a non-templated function, the compiler assumes it was emitted when the module was compiled.

This is the kind of optimization that is better fitted for link time IMHO.

@@ -309,7 +309,8 @@ void toObjFile(Dsymbol ds, bool multiobj)
override void visit(FuncDeclaration fd)
{
// in glue.c
FuncDeclaration_toObjFile(fd, multiobj);
if (fd.inlining != PINLINE.always)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if i want to always inline excepted that one time where i take its address as a function pointer ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you take the address of an inline function, you emit it locally. You certainly don't extern to an inline...

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TurkeyMan
Copy link
Contributor

TurkeyMan commented Jan 11, 2019

What about separate compilation?

I'm really confused here... what do people expect inline to do?
In the case of separate compilation:

  1. it should preferably be inserted inline at the callsite
  2. failing that for whatever reason (maybe address was taken) an instance of the function should be emit to the calling module.

Isn't this also how templates work?

If it's a non-templated function, the compiler assumes it was emitted when the module was compiled.

It's not "a non-templated function", it's a pragma(inline) function, which has had a very specific request attributed via pragma...
I think the compiler is mistaken to assume an inline function was emitted ever. That's literally the whole point... and if not that, I have absolutely no idea what the point could be?

This is the kind of optimization that is better fitted for link time IMHO.

This isn't an optimisation, it's a pragma that controls how and where code is to be emitted.

@ibuclaw
Copy link
Member

ibuclaw commented Jan 11, 2019

The semantic I'm going for in gdc is same as C extern inline. The body is always generated, regardless of which module it comes from, but it is marked as C static so will be discarded if unreferenced.

@TurkeyMan
Copy link
Contributor

@ibuclaw I think that implements the proper semantic effect.

If C implements what you describe, then I can confirm that implementation strategy works as expected:

maevans@MAEVANS2-W10:~$ cat test.cpp

inline int x() { return 10; }
int y() { return 20; }

maevans@MAEVANS2-W10:~$ gcc -S test.cpp
maevans@MAEVANS2-W10:~$ cat test.s

        .file   "test.cpp"
        .text
        .globl  _Z1yv
        .type   _Z1yv, @function
_Z1yv:
.LFB1:
        .cfi_startproc
        pushq   %rbp
        .cfi_def_cfa_offset 16
        .cfi_offset 6, -16
        movq    %rsp, %rbp
        .cfi_def_cfa_register 6
        movl    $20, %eax
        popq    %rbp
        .cfi_def_cfa 7, 8
        ret
        .cfi_endproc
.LFE1:
        .size   _Z1yv, .-_Z1yv
        .ident  "GCC: (Ubuntu 7.3.0-16ubuntu3) 7.3.0"
        .section        .note.GNU-stack,"",@progbits

This is as expected, y is emit to the object file, x is not present in the output. It is indeed the case that x is inline ;)

@TurkeyMan
Copy link
Contributor

Ah actually, thinking on it, I don't know if the C implementation applies to D verbatim.

Difference appears when you have cross-module referencing. In C, inlines are declared in headers, so the semantic you say, where they are implicitly annotated as static happens to work because #include pastes the text into each module, so each module has a local instance of the inline just by good fortune.

In D, if you have an inline in module a, and then module b references it, where does the binary end up?
A copy needs to be emit to the calling module b (to the same effect the text include would have caused in C), otherwise you'll get a link error.

Is that how GDC works?

@TurkeyMan
Copy link
Contributor

TurkeyMan commented Jan 11, 2019

lib.d
-----
module lib;
pragma(inline, true) int fun() { return 10; }
main.d
------
import lib;
int main() { return fun(); }
> gdc main.d

Do I get a link error? I expect to NOT get a link error, because the inline imported from lib should have been emit to main while compiling. That's the semantic that makes it inline. If I need to compile lib to an object file and link it, then it's totally not inline, and the whole feature is self-defeating.

C implicitly implements this semantic in conjunction with static because text-include.

@TurkeyMan
Copy link
Contributor

I guess you said "regardless of which module it comes from", so that implies that you DO emit a copy into the calling module? Maybe ignore everything I said? ;)

@TurkeyMan
Copy link
Contributor

TurkeyMan commented Jan 11, 2019

I successfully fetched gdc from the ubuntu package manager, so I ran that test on my machine, it's not correct:

maevans@MAEVANS2-W10:~$ gdc --version
gdc (Ubuntu 8-20180414-1ubuntu2) 8.0.1 20180414 (experimental) [trunk revision 259383]

maevans@MAEVANS2-W10:~$ gdc main.d -S && cat main.s | grep fun
        .type   main, @function
        .type   _Dmain, @function
        call    _D3lib3funFZi@PLT   ; <- call to extern... but where's the definition?
        .type   gdc.dso_ctor, @function
        .type   gdc.dso_dtor, @function
        .type   _GLOBAL__I_4main, @function
        .type   _GLOBAL__D_4main, @function

Compiling main.d, I can confirm that a copy of lib.fun() has not been made present in main... there is only a call to an unresolved extern. :(

maevans@MAEVANS2-W10:~$ gdc main.d
/tmp/ccc0qOXR.o: In function `_Dmain':
main.d:(.text+0x2e): undefined reference to `_D3lib3funFZi'
collect2: error: ld returned 1 exit status

No surprises there... although disappointing.

But I also noticed this, which is wrong:

maevans@MAEVANS2-W10:~$ gdc lib.d -S && cat lib.s | grep fun
        .globl  _D3lib3funFZi
        .type   _D3lib3funFZi, @function
_D3lib3funFZi:
        .size   _D3lib3funFZi, .-_D3lib3funFZi
        .type   gdc.dso_ctor, @function
        .type   gdc.dso_dtor, @function
        .type   _GLOBAL__I_3lib, @function
        .type   _GLOBAL__D_3lib, @function

Compiling lib.d on its own, a copy was output to lib despite being un-referenced. C does not do this (as I showed above), and this is undesired behaviour. I want an unreferenced inline to NOT have code emit, that's a key feature, and it's important.

TL;DR, inline doesn't work... it doesn't really do anything at all.

@ghost
Copy link

ghost commented Mar 3, 2019

lib.d
-----
module lib;
pragma(inline, true) int fun() { return 10; }
main.d
------
import lib;
int main() { return fun(); }
dmd main.d         // Link error unresolved lib.fun()
dmd -inline main.d // ok

The odd thing is that with the -inline flag it compiles fine and doesn't emit the symbol. It does without the flag though, even though there is nothing that actually needs it. The function gets inlined per pragma(inline, true) as expected. I'd expect this behavior unless it can't inline the function or when you take the address of the function, then it actually needs the symbol.

Otherwise not sure if C/C++ should be followed for inlining, part of the reason they need to be inlined in such as way is the way C++ does headers. Took a look at what rust does and I found you don't even have imported libraries in that sense, it is handled very differently.

Anyways when you generate a header for the lib function above you get the following output without the body, which is odd.

// D import file generated from 'lib.d'
module lib;
pragma (inline, true)int fun();

@TurkeyMan
Copy link
Contributor

TurkeyMan commented Mar 3, 2019

The odd thing is that with the -inline flag it compiles fine and doesn't emit the symbol

Your example works by hiding the issue. All the semantic issues I mentioned are excluded from your example above, you don't emit any object code with symbols in the wrong places.

My examples above show improper placement in object files.

@ghost
Copy link

ghost commented Mar 3, 2019

It was your example, it illustrates there's a problem when not using -inline, it shouldn't emit a symbol when it shouldn't have to. Yes it does not solve the issue that you have, but there is an issue none the less. Otherwise I trying to discuss whether it would even be a good idea to do what C++ does.

For generating code in each separate module that's what I'm saying might not be a good idea to follow in C++'s footsteps. Inline in C++ exists the way it does because there are no concepts of modules. I've been trying to find a modern language that has inlining that works in the same way that C/C++ does, but I just can't find one. It seems like that kind of behavior is an artifact of having header and source files, not just a single source file like virtually every modern language. And like I said with rust, you can't even create an object file from a single source file. It has to have the entire source available to it and it compiles everything to the object file. Which removes this problem of inlining, but obviously you have to compile the entire source. So their solution is to use things like crates to separate their code.

@TurkeyMan
Copy link
Contributor

It was your example

Well you changed my example by removing everything that my example was about (compiling with -inline, not emiting .o files, not actually inspecting the symbol table). That's not even a demo of my problem anymore... it's something orthogonal, which just happens to share the same code ;)

I don't see this as 'what C++ does', because C++ has text includes, and doesn't have modules.
I mean, there's an element of similarity, because it's a native language that emits object files that needs to link with a linker. The language is effectively gone here; it's not D anymore, the functional parts are an opaque symbol table, and a linker.

It seems like that kind of behavior is an artifact of having header and source files

I think it's an artifact of having object files and a linker.
There's a matter of common-sense when interacting with that environment. It's symptomatic of being native code. D the language doesn't/shouldn't/can't have any influence on the wider native programming environment.

WRT Rust; yes, you can see their solution is that they do whole-program compilation. And if they didn't do that, I'm certain that their inline would behave the way I'm expecting, because there's no other reasonable way for it to behave.

I don't think there's actually choices here; there's literally only one solution that works, unless you start to talk about changing D so that it must do whole program compilation, and that we don't emit .o files.

I'm also not sure why this is even a problem. Are there any issues that arise from the obvious solution? I can't think of any.

It really seems trivial; inline only emits a symbol when it's referenced, and to the calling compilation unit. That is all inline should do. It may also be a hint to the optimiser, but that's secondary to semantics that make it possible to compile and link at all.

@rainers
Copy link
Member

rainers commented Mar 4, 2019

It really seems trivial; inline only emits a symbol when it's referenced, and to the calling compilation unit. That is all inline should do.

FYI: gcc and clang don't emit the symbol with optimizations if a function is used inline only. cl does (as a COMDAT).

@ghost
Copy link

ghost commented Mar 5, 2019

Well you changed my example by removing everything that my example was about (compiling with -inline, not emiting .o files, not actually inspecting the symbol table). That's not even a demo of my problem anymore... it's something orthogonal, which just happens to share the same code ;)

Yes, it may not be the problem you illustrated anymore, but it is part of the problem that is blocking this pull request from passing.

I don't see this as 'what C++ does', because C++ has text includes, and doesn't have modules.
I mean, there's an element of similarity, because it's a native language that emits object files that needs to link with a linker. The language is effectively gone here; it's not D anymore, the functional parts are an opaque symbol table, and a linker.

The way it is implemented in C/C++ is as a result of there simply being no other alternative.

#ifndef SOME_NASTY_HEADER_GUARD_H
#define SOME_NASTY_HEADER_GUARD_H

inline int foo() {
    return 10;
}

#endif

In the above example, what anchor point could be used as a means so that C++ knows to put the actual generated code into the object file and expose the symbol for the function? There's nothing it can use, this is their problem. Their solution to this problem is that they generate code and a symbol for each translation unit as a workaround.

Now looking at D:

module noguardyay;

pragma(inline, true) void foo() {
    return 10;
}

We know the function is part of the module noguardyay so we have something we can anchor the function to. We can determine that code be generation and a symbol emited only when we are compiling the module noguardyay, which is exactly what D does. This is impossible to do with C/C++ because there is simply nothing that can be done about it. Inlining and embedding symbols from another module are two separate and distinct features. Inlining does exactly what you think, it inlines a function, there's no symbols to emit not even locally. The other feature is an artifact of C/C++ and there simply being no other workaround. Had it been implemented differently, then C/C++ probably wouldn't be using headers with text substitution and would be something different in place.

Anyways back to the problem at hand. You are trying to stop the C++ symbol from being included as part of phobos, so that it can still be used without having to link to the C++ runtime. You can just use a template for that, it does pretty much the exact same thing. No symbol or code generation happens unless it is instigated by being called somewhere. The other issue you were having with nothrow is more or less D's mistake in allowing you to define two functions that only different because of the nothrow attribute. And a bug that allowed you to call those functions in a way that shouldn't have been possible.

WRT Rust; yes, you can see their solution is that they do whole-program compilation. And if they didn't do that, I'm certain that their inline would behave the way I'm expecting, because there's no other reasonable way for it to behave.

I'm not so sure, though that's more in the fact that they probably won't ever implement it that way in the first place. But anyways I still can't find any other language that does, it seems to be more or less unique to C/C++. I've given up on my search, I'd be welcome to be proven wrong though :P.

It really seems trivial; inline only emits a symbol when it's referenced, and to the calling compilation unit. That is all inline should do. It may also be a hint to the optimiser, but that's secondary to semantics that make it possible to compile and link at all.

What if there's an embedded global variable inside? Should that be followed with C++ as well? If we do follow C++ then depending on how it is compiled those types will reference different global variables. It then just becomes much more complicated with different behavior based on how it is compiled, and down the rabbit hole we go...

@TurkeyMan
Copy link
Contributor

FYI: gcc and clang don't emit the symbol with optimizations if a function is used inline only. cl does (as a COMDAT).

Right, it is at the discretion of the caller whether to emit a symbol to the local CU or not. If they inline it and don't emit a symbol, that is fine and proper.

@TurkeyMan
Copy link
Contributor

In the above example, what anchor point could be used as a means so that C++ knows to put the actual generated code into the object file and expose the symbol for the function? There's nothing it can use, this is their problem. Their solution to this problem is that they generate code and a symbol for each translation unit as a workaround.

I mean, the thing you say is true, but I don't think the reason is necessary to explain it; it's just that it naturally works out that way, and in C++, it happens to match the conceptual model you suggest.
I suspect the conceptual model is just correlation, not necessarily causation. It's just that that's the only design that makes sense in a native-language world.
I mean, C/C++ had some influence on the development of the native language ecosystem, but native binaries are still what they are after the compiler's have finished with them and the language is long gone from the symbol table.

Now looking at D:

module noguardyay;

pragma(inline, true) void foo() {
    return 10;
}

We know the function is part of the module noguardyay so we have something we can anchor the function to.

** something to anchor the function declaration to. This is in the world of the languages AST, and not very relevant to the conversation.
D modules have no relationship to the output CU's.

We can determine that code be generation and a symbol emited only when we are compiling the module noguardyay, which is exactly what D does.

Right... and that's the whole problem here!

This is impossible to do with C/C++ because there is simply nothing that can be done about it.

I'm not sure what C++ has to do with this behaviour. It's a different language with a totally different thing going on in this respect. Although the output is indeed the same, a flat symbol table in an object file that gets collated by the linker.

Even if this could be done in C++, why would you do it? It would be broken, just like it is in D!

Inlining and embedding symbols from another module are two separate and distinct features. Inlining does exactly what you think, it inlines a function, there's no symbols to emit not even locally.

Right, and this is fine, assuming the inlining does take place. It's at the discretion of the compiler to actually inline a thing. If it chooses not to, it needs to emit the symbol.

The other feature is an artifact of C/C++ and there simply being no other workaround. Had it been implemented differently, then C/C++ probably wouldn't be using headers with text substitution and would be something different in place.

This point is weird... it has nothing to do with C++, I'm not sure why you make that leap?
Even in C++'s case, you seem to suggest that the behaviour doesn't make perfect sense, and that they'd do something else if they could?
Why would they? The behaviour is perfectly sensible, and the only reasonable option for any native language that doesn't do all-at-once builds (which includes D).

This is an artifact of the compiler choosing not to inline the function. It's at the discretion of the compiler to inline or not, and if not, it may choose to emit a hard copy locally. There are reasons why this may occur; perhaps you take the address of the function? Heuristics determine that it's a disadvantage to inline it, etc.

I don't see how or why language has anything to say about this... there's no other arrangement I can imagine that works.

Anyways back to the problem at hand. You are trying to stop the C++ symbol from being included as part of phobos,

It's just one problem that the design manifests in this case.
I have encountered numerous other scenarios, particularly in very modular code.

In this case, we certainly don't want a hard-reference to a foreign library in druntime, which leads to a guaranteed link error because code that makes a hard-call to a lib appears in the lib, and the linker wants to resolve the symbol. This is a trivial wrapper, it should be inline, and the code should be generated only if it's ever called.
I specifically do not want a hard-copy of the function to appear in any symbol table. Hence inline.

so that it can still be used without having to link to the C++ runtime. You can just use a template for that, it does pretty much the exact same thing.

Right, that's the lame workaround because inline's broken. I just want an inline, I do not want a template, template instantiations, template overload semantics, etc. This should be fine.
It's legitimate to want inline semantics, and to not want a template.

Why use a hacky workaround, when inline should just do what's written on the tin?
Fortunately, it's not even difficult or weird for D; the desired semantics are already present in the compiler! They're just not applied.

What's a disadvantage to fixing this? It will just make certain classes of problem better. I'm pretty sure there are no disadvantages.
And if inline doesn't do that... what on earth does it to? Currently does nothing other than a mild hint to the optimiser to "please inline a thing even if you'd prefer not to, but you don't have to if you don't want to".
But that really misses the whole point! And that semantic just comes naturally when it's defined to be generally useful.

WRT Rust; yes, you can see their solution is that they do whole-program compilation. And if they didn't do that, I'm certain that their inline would behave the way I'm expecting, because there's no other reasonable way for it to behave.

I'm not so sure, though that's more in the fact that they probably won't ever implement it that way in the first place. But anyways I still can't find any other language that does, it seems to be more or less unique to C/C++. I've given up on my search, I'd be welcome to be proven wrong though :P.

What other languages have you considered?
There's not very many native languages to search through.

Pascal certainly works this way. I tested Crystal, that does too.
Fortran doesn't have any inline request.

I struggled to make Go and Nim just compile a single source file into a .o file to test, they both seem to have complex projects and build environments going on.

It really seems trivial; inline only emits a symbol when it's referenced, and to the calling compilation unit. That is all inline should do. It may also be a hint to the optimiser, but that's secondary to semantics that make it possible to compile and link at all.

What if there's an embedded global variable inside?

Do we support inline variables? I think we don't. That's a tangent conversation.

Should that be followed with C++ as well?

We're not talking about C++.

If we do follow C++ then depending on how it is compiled those types will reference different global variables. It then just becomes much more complicated with different behavior based on how it is compiled, and down the rabbit hole we go...

We're not talking about global variables. It's hard to know what an inline 'global' variable even means?
You'll get a link error for the unresolved reference to the global variable like you expect. I can't see the rabbit hole.

@ghost
Copy link

ghost commented Mar 6, 2019

** something to anchor the function declaration to. This is in the world of the languages AST, and not very relevant to the conversation.
D modules have no relationship to the output CU's.

It's a matter of where the function generated code should be placed so that it gets linked to correctly. If it can only be generated in one place then you don't have to worry about there being two of the functions. In C++ you can easily define the same function in multiple source files and get a linker error. Doing that in D isn't really possible and that in part is because of the module system. The structure of the language prevents what would otherwise be a linker error.

It's just one problem that the design manifests in this case.
I have encountered numerous other scenarios, particularly in very modular code.

I'd be interested to see some examples of these.

Right, that's the lame workaround because inline's broken. I just want an inline, I do not want a template, template instantiations, template overload semantics, etc. This should be fine.
It's legitimate to want inline semantics, and to not want a template.

For the most part it does the same thing. A template instantiation would be similar to what you get with code being generated per module, and is only generated when it is used. What are you doing with it other than calling the function that makes it undesirable?

What's a disadvantage to fixing this? It will just make certain classes of problem better. I'm pretty sure there are no disadvantages.
And if inline doesn't do that... what on earth does it to? Currently does nothing other than a mild hint to the optimiser to "please inline a thing even if you'd prefer not to, but you don't have to if you don't want to".
But that really misses the whole point! And that semantic just comes naturally when it's defined to be generally useful.

If there was even an inline feature in C++ that did that, rather than __inline, __inline__, __forceinline, __attribute__((always_inline)), what other compiler specific attributes are there... I still feel that what inline does in C++ and what people actually want from it are two different things. If we implemented it the way you want, what would we use instead if we just wanted it to inline a function?

Pascal certainly works this way. I tested Crystal, that does too.
Fortran doesn't have any inline request.

Pascal is almost as old as C++, same goes for Fortran. Kind of odd it doesn't have an inline feature.

I struggled to make Go and Nim just compile a single source file into a .o file to test, they both seem to have complex projects and build environments going on.

That's what I've found too, modern native languages are doing away with .o files because of the problems they cause.

We're not talking about global variables. It's hard to know what an inline 'global' variable even means?
You'll get a link error for the unresolved reference to the global variable like you expect. I can't see the rabbit hole.

You don't get a linker error in C++ though.

// foo.h

inline int& foo() 
{
    static int value;
    return value;
}

If you use this header file in A.dll, B.dll and main.exe all linked together, there will be 3 instances of static int value. With D right now, this is not a problem. If you attempted to do this you will get link errors that either it doesn't exist or that there are duplicates. That is assuming you compiled the D source file 3 times and not just compiling it once and importing it for the other two like what you should do. It maintains there only being a single instance.

@RazvanN7
Copy link
Contributor

I'm going to close this due to lack of activity. The PR is linked in the bug report so anyone that wishes to continue work on this PR can get it from there.

@RazvanN7 RazvanN7 closed this Apr 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants