Skip to content
This repository was archived by the owner on Oct 12, 2022. It is now read-only.
/ druntime Public archive

Add support for native TLS on OS X#1461

Merged
yebblies merged 1 commit intodlang:masterfrom
jacob-carlborg:native_tls
Mar 24, 2016
Merged

Add support for native TLS on OS X#1461
yebblies merged 1 commit intodlang:masterfrom
jacob-carlborg:native_tls

Conversation

@jacob-carlborg
Copy link
Contributor

Runtime part of dlang/dmd#5346.

@jacob-carlborg
Copy link
Contributor Author

Added back emulate TLS for 32bit.

Choose a reason for hiding this comment

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

sorry if this is a nitpick..but couldn't you just use public import instead of a mixin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I did it like this is because the code in sections_osxXX depends on code in sections_osx. I tried to avoid a circular dependency without changing too much of the code. Not sure if this is better though.

Choose a reason for hiding this comment

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

I probably would have opted for creating a third/common file. The mixin approach has two problems: The mixin could change meaning depending on where it's used, and while editing the contents of the mixin, intellisense systems will not know where to look for the symbols. Newcomers looking at sections_osxXX may also be confused as well. I'm not sure how prevalent this type of thing is in the druntime code, but if it can be avoided, I think it should be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So you mean a fourth file? There's already section_osx.d, section_osx32.d and section_osx64.d.

I'll wait for some additional input before making any changes.

Choose a reason for hiding this comment

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

Actually, taking a fresh look at things this morning, these three files are so short that I don't think it really makes sense to split them up. Can we just keep it all in one file?

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 guess that could work. I would still like some input form someone else as well.

Choose a reason for hiding this comment

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

Ok, but I strongly believe that any of the solutions that I've suggested will have us in a better spot than a mixin.

Choose a reason for hiding this comment

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

Actually, Looking at my code for shared libraries, I would have to change the SectionGroup struct and the onAddImage function as well. And per Walter's response in the forums, shared libraries should be made available for 64bit only. So this eliminates/differentiates most of what's still being shared between the two files sections_osx32.d and sections_osx64.d.

So could we go with my last suggestion, and have one file for each platform, and replace the imports in sections.d with this?

else version(OSX)
{
    version(D_LP64)
        public import rt.sections_osx64
    else
        public import rt.sections_osx32
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bitwise-github I've updated the code now, structured according to your suggestion.

Choose a reason for hiding this comment

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

Ok, thanks.

@andralex
Copy link
Member

@jacob-carlborg
Copy link
Contributor Author

@andralex this PR for druntime is required #1461.


void _d_dyld_getTLSRange(void* arbitraryTLSSymbol, void** start, size_t* size) {
dyld_enumerate_tlv_storage(
^(enum dyld_tlv_states state, const dyld_tlv_info *info) {
Copy link
Member

Choose a reason for hiding this comment

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

Please don't use weird vendor extensions, for sure you can do it with a plain callback or a struct.

Copy link
Contributor

Choose a reason for hiding this comment

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

The API is only offered using a blocks-based callback. The need to do that is in fact the only reason why the code exists in an extra .c file. I briefly considered reverse-engineering the Blocks ABI to use it from D, but just using it the way it is supposed to seems like the much easier and more robust solution.

@jacob-carlborg
Copy link
Contributor Author

New PR is: #1523

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants