Skip to content

Comments

Add ability to use interfaces and classes without the runtime if they only contain static members#8204

Merged
dlang-bot merged 1 commit intodlang:masterfrom
JinShil:minimal_runtime_with_classes
Apr 27, 2018
Merged

Add ability to use interfaces and classes without the runtime if they only contain static members#8204
dlang-bot merged 1 commit intodlang:masterfrom
JinShil:minimal_runtime_with_classes

Conversation

@JinShil
Copy link
Contributor

@JinShil JinShil commented Apr 24, 2018

This is a followup to the PRs previously released in 2.079 that enable users to use D without the runtime in a pay-as-you-go fashion

Currently, even with the above PRs, the following code will cause the compiler to emit an error about TypeInfo if compiled without the runtime. However, TypeInfo is in no way required for such code because all interface/class members are static.

interface I
{
    static int i;
}

class A : I
{
    static int a;
}

class B : A
{
    static int b;
}

void main() 
{ 
    B.i = 32;
    B.a = 42;
    B.b = 52;
}

This PR removes this arbitrary limitation enabling more features of D to be used without the runtime.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @JinShil!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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#8204"

@JinShil JinShil changed the title Add ability to use classes without the runtime if they only contain static members Add ability to use interfaces and classes without the runtime if they only contain static members Apr 24, 2018
@jacob-carlborg
Copy link
Contributor

There might some confusion around which part of a feature can be used where.

@JinShil
Copy link
Contributor Author

JinShil commented Apr 26, 2018

@jacob-carlborg Are you a Mac user? If so, any chance I can persuade you to look into the Darwin failure? I'm mostly concerned with why it behaves differently from Linux, and would like to reduce the discrepancies.

@jacob-carlborg
Copy link
Contributor

I’ll have a look.

@jacob-carlborg
Copy link
Contributor

jacob-carlborg commented Apr 26, 2018

@JinShil macOS 32-bit is using a custom implementation of TLS. For every access to a TLS variable, the compiler generates a call to the ___tls_get_addr function which is defined in druntime. The -defaultlib= flag will cause druntime to not be linked meaning this symbol won't exist.

This does not occur on macOS 64-bit since it's using the system provided functionality for TLS (implemented in the dynamic loader). The reason why it's not used on 32-bit is because it did not work out of the box (I don't see a reason why it wouldn't, they both use the same system). Because macOS 32-bit is a dead platform we felt it wasn't worth spending time trying to fix the issue.

I suspect it's a simple fix to get the native TLS working on 32-bit, once someone manage to find the issue.

@JinShil
Copy link
Contributor Author

JinShil commented Apr 26, 2018

Thank you @jacob-carlborg, very helpful. I've disabled the runnable test for osx32 and added a compilable test. See my comments in the test files for justification.

@rainers Do you have any insight to offer for the Win32_64 error? The exit code is 5, which, based on what I could find, is an access denied error.

@jacob-carlborg
Copy link
Contributor

jacob-carlborg commented Apr 26, 2018

I've disabled the runnable test for osx32 and added a compilable test.

@JinShil an alternative would be to not use TLS variables, or do you want to explicitly test that?

Or a third option. Add the implementation of ___tls_get_addr inline, like this:

extern(D) void* ___tls_get_addr( void* p )
{
    return p;
}

@JinShil
Copy link
Contributor Author

JinShil commented Apr 26, 2018

an alternative would be to not use TLS variables, or do you want to explicitly test that?

I specifically want to test TLS members as I think that is the most common usage.

@rainers
Copy link
Member

rainers commented Apr 26, 2018

@rainers Do you have any insight to offer for the Win32_64 error? The exit code is 5, which, based on what I could find, is an access denied error.

@JinShil 5 seems an abbreviation of 0xc0000005 which is an access violation. The crash happens in the B.a = 42 assignment:

    39:     B.i = 32;
00007FF704BC1087 B8 20 00 00 00       mov         eax,20h  
00007FF704BC108C 65 48 8B 0C 25 58 00 00 00 mov         rcx,qword ptr gs:[58h]  
00007FF704BC1095 48 8B 11             mov         rdx,qword ptr [rcx]  
00007FF704BC1098 BB 00 00 00 00       mov         ebx,0  
    38: { 
    39:     B.i = 32;
00007FF704BC109D 89 04 1A             mov         dword ptr [rdx+rbx],eax  
    40:     B.a = 42;
00007FF704BC10A0 BE 2A 00 00 00       mov         esi,2Ah  
00007FF704BC10A5 65 48 8B 3C 25 58 00 00 00 mov         rdi,qword ptr gs:[58h]  
00007FF704BC10AE 48 8B 0F             mov         rcx,qword ptr [rdi]  <---- crashes here
00007FF704BC10B1 BA 04 00 00 00       mov         edx,4  
00007FF704BC10B6 89 34 11             mov         dword ptr [rcx+rdx],esi  

TLS seems broken: the first entry points to the tls_array table itself, so the assignemnt before overwrites the pointer. I guess it misses some of the magic from druntime/src/rt/msvct_stub.c

Edit: sorry, that file only exists on my disk, it has made its way into the installer repo as https://github.com/dlang/installer/blob/build-mingw-libs/windows/mingw/msvcrt_data.c

@JinShil JinShil closed this Apr 27, 2018
@JinShil JinShil reopened this Apr 27, 2018
@JinShil
Copy link
Contributor Author

JinShil commented Apr 27, 2018

@jacob-carlborg and @rainers, Thank you for your help.

OSX32 and Windows have at least part of their TLS implementation in the D runtime, so if users opt into using TLS variables and are not using a runtime, or creating their own runtime, they will need to link to an existing TLS implementation or build their own. I've, therefore, moved the TLS test to the compilable folder and modified the runnable test so it executes only for shared static members, no longer requiring a TLS implementation.

I would prefer to emit a compile-time error if a TLS implementation is not found, but that would require reworking a lot of code, and probably be too disruptive to support this niche use case.

With this PR, users will at least no longer be obstructed with an irrelevant TypeInfo error.

It's all green now, and ready to go..

Copy link
Contributor

@jacob-carlborg jacob-carlborg left a comment

Choose a reason for hiding this comment

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

There’s no changelog entry.


// This test ensures that interfaces and classes can be used in a minimal
// runtime as long as they only contain shared static members. Non-shared
// static members would a thread-local storage (TLS) implementation.
Copy link
Contributor

Choose a reason for hiding this comment

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

The last sentence sounds a bit weird. Is it missing a “required”?

@jacob-carlborg jacob-carlborg added the Review:Needs Changelog A changelog entry needs to be added to /changelog label Apr 27, 2018
@rainers
Copy link
Member

rainers commented Apr 27, 2018

The necessary TLS magic for Windows is in the C runtime, too. What is missing is a reference to the respective symbols, but it is optimized away under the assumption that the code ends up in the executable image. If you compile it separately, _tls_index will be used and the code should be fine. The test than fails due to missing _d_run_main, though.

@jacob-carlborg
Copy link
Contributor

The test than fails due to missing _d_run_main, though.

That symbol is available in the runnable/extra-files/minimal/object.d file, which is included in the test as well [1].

[1] https://github.com/dlang/dmd/pull/8204/files#diff-26f06e2cdfaed27b14b8a004281f0798R2

@JinShil
Copy link
Contributor Author

JinShil commented Apr 27, 2018

That symbol is available in the runnable/extra-files/minimal/object.d file, which is included in the test as well

Right, so using that object.d you should be able to just do dmd -c main.d object.d to compile separately, and then link. But I have no idea what the linker command should be for Windows.

@jacob-carlborg
Copy link
Contributor

For macOS 32-bit you can add a dummy implementation:

extern(D) void* ___tls_get_addr( void* p )
{
    return p;
}

It won't be a true TLS but you're not using threads so...

@rainers
Copy link
Member

rainers commented Apr 27, 2018

I have just added COMPILE_SEPARATELY to the test, but that didn't pick up object.obj (for obvious reasons). There doesn't seem to be a simple way to add it to the link command line.

@JinShil
Copy link
Contributor Author

JinShil commented Apr 27, 2018

@jacob-carlborg and @rainers, are you concerned about having a runnable test for osx32 and Windows?

@jacob-carlborg
Copy link
Contributor

are you concerned about having a runnable test for osx32

No.

@JinShil
Copy link
Contributor Author

JinShil commented Apr 27, 2018

I have just added COMPILE_SEPARATELY to the test, but that didn't pick up object.obj (for obvious reasons). There doesn't seem to be a simple way to add it to the link command line.

You add the _d_run_main implementation directly to the minimal2.d file and it should work then. But still, I'm not sure how to link once the .obj file is created.

@rainers
Copy link
Member

rainers commented Apr 27, 2018

You add the _d_run_main implementation directly to the minimal2.d file and it should work then

Yes, that works with COMPILE_SEPARATELY and runs successfully.

@rainers
Copy link
Member

rainers commented Apr 27, 2018

are you concerned about having a runnable test for osx32 and Windows?

Not really ;-)

)

Non-shared static members can also be used, but will require a thread-local storage (TLS) implementation.
For Linux, the TLS implementation is already supplied by the C runtime and C
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we know that there are only two platforms where it doesn't work out of the box?

Copy link
Contributor Author

@JinShil JinShil Apr 27, 2018

Choose a reason for hiding this comment

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

All the platforms in the auto-tester passed except osx32 and Win32_64. So those are the only supported platforms I know of that don't work out of the box.

Copy link
Contributor

Choose a reason for hiding this comment

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

Exactly, so why only mentioning Linux?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm just using Linux and an illustrative example. How else should I say it? "For *nix platforms..."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add "For example.."

@jacob-carlborg jacob-carlborg removed the Review:Needs Changelog A changelog entry needs to be added to /changelog label Apr 27, 2018
@jacob-carlborg
Copy link
Contributor

Do we want/is there a need to test static methods as well?

@JinShil
Copy link
Contributor Author

JinShil commented Apr 27, 2018

Do we want/is there a need to test static methods as well?

Added a method to the runtime test.

@dlang-bot dlang-bot merged commit 40b18f0 into dlang:master Apr 27, 2018
@12345swordy
Copy link
Contributor

Wait hold on, does this mean it's possible for determine the attributes for destroy at compile time because of this?

@JinShil
Copy link
Contributor Author

JinShil commented Apr 29, 2018

Wait hold on, does this mean it's possible for determine the attributes for destroy at compile time because of this?

I don't see how it would.

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.

6 participants