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

Initial Mac OSX Support #117

Merged
merged 9 commits into from Feb 7, 2015

Conversation

Projects
None yet
3 participants
@kangaroo
Contributor

kangaroo commented Feb 7, 2015

Supports building up the VM, PAL and various components on Mac OSX 10.10
There are some oddities with the Apple assembler not generating short jumps
and not supporting 1 byte relocs.

This is a continuation of #105 since GitHub cannot detect my new squashed commit.

Initial Mac OSX Support
Supports building up the VM, PAL and various components on Mac OSX 10.10
There are some oddities with the Apple assembler not generating short jumps
and not supporting 1 byte relocs.

@kangaroo kangaroo referenced this pull request Feb 7, 2015

Closed

MacOSX Support #105

Fixup a few changes missed when rebasing OSX support
Initially __LINUX__ was defined when building for OSX as well, bringing
in LONGDOUBLE_IS_DOUBLE 1, and some of the local SEH funcations.
Add a __APPLE__ guard around these as well, so they're properly included.
@kangaroo

This comment has been minimized.

Contributor

kangaroo commented Feb 7, 2015

The .byte issue is still present that @janvorli pointed out.

I tried the .macro suggestion, but OS X generated a reloc, and the smallest reloc size they support is 3 bytes. I'm open to discussion other possibilities.

Perhaps its best to let OS X pad the jumps out, and fix the calculation on the patching side?

@janvorli

This comment has been minimized.

Member

janvorli commented Feb 7, 2015

Few details:

  1. You can replace the following
#if defined(__APPLE__)
    static const char * const coreClrDll = "libcoreclr.dylib";
#else
    static const char * const coreClrDll = "libcoreclr.so";
#endif

by

static const char * const coreClrDll = MAKEDLLNAME_A("coreclr");
  1. Could you please rename the arch/i386/context.clang.s to context.S?
  2. What is the reason for using clang instead of llvm? Is only the former defined on Mac? We use llvm at other places.
  3. The non-clang version of DBG_CheckStackAlignment() is now missing the asm prefix before the "VOID". I know it is not being built now, but I'd keep it there.
  4. Something needs to be done about the macro parameters. We cannot just pass the Handler when there should be no handler.
@janvorli

This comment has been minimized.

Member

janvorli commented Feb 7, 2015

I am sorry, github has renamed the clang and llvm in my previous post

@janvorli

This comment has been minimized.

Member

janvorli commented Feb 7, 2015

Regarding the short jump issue, I just got an idea which probably won't change anything, but still is worth trying. Could you please try to replace the labels of the jumps with numeric labels and put the respective numeric label to the place where the jump should land? Maybe from the fact that these are short range labels, the assembler would decide to put in the short versions.
Like this:

PATCH_LABEL JIT_WriteBarrier_PreGrow32_PatchLabel_CardTable_Check
        cmp     byte ptr [rdi + 0F0F0F0F0h], 0FFh
        jne     1f
        REPRET

        nop // padding for alignment of constant
1:
PATCH_LABEL JIT_WriteBarrier_PreGrow32_PatchLabel_CardTable_Update
    UpdateCardTable_PreGrow32:
@kangaroo

This comment has been minimized.

Contributor

kangaroo commented Feb 7, 2015

  1. MAKEDLLNAME_A seems to only be available in the PAL, is it ok to add pal.h to the corerun.cpp? I only ask because so far nothing seems to include pal.h outside the pal.
    2, 3, 4) Fixed in d288813
  2. I'm open to anything, the only reason I was passing it as Handler right now is its completely unused in NESTED_ENTRY
@kangaroo

This comment has been minimized.

Contributor

kangaroo commented Feb 7, 2015

With the short jump issue, numeric lables (or text) are fine for jumping without a reloc, but it goes back to generating the long jumps.

@janvorli

This comment has been minimized.

Member

janvorli commented Feb 7, 2015

Regarding 1), you're right, I've missed the fact that it is in the corerun.cpp. So please leave it as it is.
As for 5), let me try to figure out something over the weekend.

@janvorli

This comment has been minimized.

Member

janvorli commented Feb 7, 2015

Hmm, so it seems to me that maybe we should let the assembler use the long versions and just put in more / less nops for Mac to achieve the proper alignment of the constants in instructions following the PATCH_LABEL,
The 32 bit constants need to be aligned on 4 bytes, the 64 bit constants on 8 bytes, as you have probably expected.

@kangaroo

This comment has been minimized.

Contributor

kangaroo commented Feb 7, 2015

I'm happy to make the change that way. One quick question, the fast write barriers appear to use PATCH_LABEL to find the constants, but JIT_WriteBarrier does not. Can you point me to where its patch offsets are calculated so I can fit it up as well?

@janvorli

This comment has been minimized.

Member

janvorli commented Feb 7, 2015

This comment at the JIT_WriteBarrier should explain that:

// This is used by the mechanism to hold either the JIT_WriteBarrier_PreGrow 
// or JIT_WriteBarrier_PostGrow code (depending on the state of the GC). It _WILL_
// change at runtime as the GC changes. Initially it should simply be a copy of the 
// larger of the two functions (JIT_WriteBarrier_PostGrow) to ensure we have created
// enough space to copy that code in.

Currently it is a copy of the JIT_WriteBarrier_PreGrow64, so if you make changes to JIT_WriteBarrier_PreGrow64, you should make the same ones to the JIT_WriteBarrier.

@jkotas

This comment has been minimized.

Member

jkotas commented Feb 7, 2015

The write barriers are the most performance sensitive helpers. It would be really nice to keep them on the short jump plan, so that they are as small as possible. I think that the jumps emitted via hardcoded bytes is the best solution - as it is right now - is the best solution among the ones discussed.

build.sh Outdated
#determine nproc in a platform independent way
NPROCS=1
OS=`uname`

This comment has been minimized.

@jkotas

jkotas Feb 7, 2015

Member

This should not be needed anymore

This comment has been minimized.

@kangaroo

kangaroo Feb 7, 2015

Contributor

Fixed in 854c2ae

#ifndef __llvm__
asm
VOID
DBG_CheckStackAlignment()

This comment has been minimized.

@jkotas

jkotas Feb 7, 2015

Member

This version can be just deleted now that we have a proper one in .S file.

This comment has been minimized.

@kangaroo

kangaroo Feb 7, 2015

Contributor

Fixed in 13c61fd

@@ -41,7 +41,9 @@ Module Name:
#include <string.h>
#include <unistd.h>
#include <pthread.h>
#if defined(__powerpc__)

This comment has been minimized.

@jkotas

jkotas Feb 7, 2015

Member

ppc is dead code - it can be just deleted

This comment has been minimized.

@kangaroo

kangaroo Feb 7, 2015

Contributor

Fixed in a010cbd

@@ -359,7 +359,7 @@ static CFStringRef CFStringCreateMacFormattedLocaleName(LPCWSTR lpLocaleName)
return NULL;
}
CFStringAppendCharacters(cfMutableLocaleName, lpLocaleName, PAL_wcslen(lpLocaleName));
CFStringAppendCharacters(cfMutableLocaleName, (const UniChar*)lpLocaleName, PAL_wcslen(lpLocaleName));

This comment has been minimized.

@jkotas

jkotas Feb 7, 2015

Member

Could you please create UniChar * ToUniChar(WCHAR * ) helper method (or operator) and use it instead of the hard casts, so that we still get the types checked by the compiler?

This comment has been minimized.

@kangaroo

kangaroo Feb 7, 2015

Contributor

Not sure I follow, you mean something like this:

https://gist.github.com/kangaroo/95ba61cf32338a4d5368

This comment has been minimized.

@jkotas

jkotas Feb 7, 2015

Member

Yes, it is exactly what I meant.

This comment has been minimized.

@kangaroo

kangaroo Feb 7, 2015

Contributor

Thanks, fixed in ffccc3a

@@ -94,7 +94,7 @@ LEAF_END HelperMethodFrameRestoreState, _TEXT
// generation will need to altered accordingly.
//
// EXTERN_C VOID __stdcall NDirectImportThunk()//
NESTED_ENTRY NDirectImportThunk, _TEXT
NESTED_ENTRY NDirectImportThunk, _TEXT, Handler

This comment has been minimized.

@jkotas

jkotas Feb 7, 2015

Member

To deal with the empty macro arg problem, I would use something more distinct like "NoHandler". Once/if the argument is used by the macro, the distinct name can be special cased.

This comment has been minimized.

@kangaroo

kangaroo Feb 7, 2015

Contributor

Fixed in e1581f6

@jkotas

This comment has been minimized.

Member

jkotas commented Feb 7, 2015

LGTM, modulo the feedback above

@kangaroo

This comment has been minimized.

Contributor

kangaroo commented Feb 7, 2015

I believe that addresses everything (again 👍)

Would you like me to squash again, or is it ok as is?

@jkotas

This comment has been minimized.

Member

jkotas commented Feb 7, 2015

LGTM

jkotas added a commit that referenced this pull request Feb 7, 2015

@jkotas jkotas merged commit bb38f10 into dotnet:master Feb 7, 2015

1 check passed

default Merged build finished.
Details
@jkotas

This comment has been minimized.

Member

jkotas commented Feb 7, 2015

Thanks a lot!

@kangaroo

This comment has been minimized.

Contributor

kangaroo commented Feb 7, 2015

My pleasure, thanks for the quick merge and review.

On to fixing more tests now 👍

@janvorli

This comment has been minimized.

Member

janvorli commented Feb 9, 2015

@kangaroo I got one idea about the assembler on clang complaining about the missing macro parameters. Could you please try to add "=" to the Handler parameter to indicate the parameter has a default value that is empty as follows:
.macro NESTED_ENTRY Name, Section, Handler=
and see if that allows omitting the parameter when using the macro?
Clang on my Ubuntu accepts that syntax, so I hope this could work on OSX as well.

@kangaroo

This comment has been minimized.

Contributor

kangaroo commented Feb 9, 2015

Nope:

/var/folders/_s/vb2gh7xn20l2wrvvlrf7mpd80000gn/T/virtualcallstubamd64-dd3c5b.s:281:41: error: macro argument 'Handler' is missing
NESTED_ENTRY ResolveWorkerAsmStub, _TEXT

@janvorli

This comment has been minimized.

Member

janvorli commented Feb 9, 2015

Ok, thanks for testing it.

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