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

cross-platform: Make amd64 asm files compile on OSX #1131

Merged
merged 1 commit into from
Jun 14, 2016

Conversation

tadeuzagallo
Copy link
Contributor

The assembly files fail to assemble due to unsupported @plt modifier and fail to link on OS X due to missing label prefix on function names.

@msftclas
Copy link

Hi @tadeuzagallo, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;


#ifndef __APPLE__
.type _ZN2Js18JavascriptFunction20DeferredParsingThunkEPNS_16RecyclableObjectENS_8CallInfoEz, @function
.type _ZN2Js18JavascriptFunction24DeferredDeserializeThunkEPNS_16RecyclableObjectENS_8CallInfoEz, @function
.type C_FUNC(_ZN2Js18JavascriptFunction20DeferredParsingThunkEPNS_16RecyclableObjectENS_8CallInfoEz), @function
Copy link
Collaborator

Choose a reason for hiding this comment

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

C_FUNC is not needed here

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 know it's not currently necessary, but it relies on an assumption of how the compiler mangles the symbols, so I usually prefer to keep it for consistency. Of course I can remove it though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@tadeuzagallo pseudo-op .type is not supported by OSX target anyways.

@obastemur
Copy link
Collaborator

@tadeuzagallo IMO these files need a bit more change to be OSX compatible or to be able to pass the linker.

However, I'm okay with the changes overall. Please update the title from compatible with to compiles on, and squash your changes.

Thanks for the PR!

@tadeuzagallo tadeuzagallo changed the title cross-platform: Make amd64 asm files compatible with OSX cross-platform: Make amd64 asm files compile on OSX Jun 14, 2016
@obastemur
Copy link
Collaborator

LGTM. @tadeuzagallo Thanks for the PR again

@chakrabot chakrabot merged commit d6f9f57 into chakra-core:linux Jun 14, 2016
chakrabot pushed a commit that referenced this pull request Jun 14, 2016
Merge pull request #1131 from tadeuzagallo:linux
The assembly files fail to assemble due to unsupported `@plt` modifier and fail to link on OS X due to missing label prefix on function names.
chakrabot pushed a commit that referenced this pull request Jun 18, 2016
Merge pull request #1134 from obastemur:osx_support
Disclaimer: Currently, ChakraCore is not officially supported on OSX.

This PR targets the individuals interested with experimenting ChakraCore on
OSX. After seeing #1131, spent couple of hours to fix the compile, and link
time issues on OSX target.

Steps to compile :
```
brew install icu4c
./build.sh --debug -j:2 --icu=/usr/local/Cellar/icu4c/54.1/include/
```

Status:
CMAKE has an issue with shared bundles on OSX (a.k.a. so) and currently CH does not
play well with dylib.
obastemur added a commit to obastemur/ChakraCore that referenced this pull request Jun 24, 2016
Disclaimer: Currently, ChakraCore is not officially supported on OSX.

This PR targets the individuals interested with experimenting ChakraCore on
OSX. After seeing chakra-core#1131, spent couple of hours to fix the compile, and
link time issues on OSX target.

Steps to compile :

```
brew install icu4c
./build.sh --debug -j:2 --icu=/usr/local/Cellar/icu4c/54.1/include/
```

Status:
CMAKE has an issue with OSX shared bundles and currently CH does not
play well with dylib output.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants