[x86/Linux] add three functions for _X86_TARGET_ by ragmani · Pull Request #10378 · dotnet/coreclr

ragmani · 2017-03-22T08:58:46Z

There is no functions for X86_TARGET.

ZapUnwindData::GetAlignment()
DWORD ZapUnwindData::GetSize()
void ZapUnwindData::Save(ZapWriter * pZapWriter)

dnfclas · 2017-03-22T08:58:50Z

@ragmani,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

ragmani · 2017-03-22T09:00:39Z

@parjong This PR is from #10139

parjong · 2017-03-22T11:24:10Z

+    dwSize = AlignUp(dwSize, sizeof(ULONG));
+
+    dwSize += sizeof(ULONG);
+#endif //REDHAWK


While working on #8889, I overhear that x86 has no personality routine space (please refer to reservePersonalityRoutineSpace in vm/jitinterface.cpp), but I'm not sure whether the line should be aligned with that, or not.

@jkotas @janvorli Could you correct me if there is something wrong?

Yes, this should do the same thing as reservePersonalityRoutineSpace in VM

parjong · 2017-03-22T23:53:03Z

@dotnet-bot test Windows_NT x86 Checked Build and Test please

dnfclas · 2017-03-24T07:31:30Z

@ragmani,
Thanks for your contribution.
To ensure that the project team has proper rights to use your work, please complete the Contribution License Agreement at https://cla2.dotnetfoundation.org.

It will cover your contributions to all .NET Foundation-managed open source projects.
Thanks,
.NET Foundation Pull Request Bot

There is no functions for _X86_TARGET_. - ZapUnwindData::GetAlignment() - DWORD ZapUnwindData::GetSize() - void ZapUnwindData::Save(ZapWriter * pZapWriter)

remove creating personaly routine when assemblies is generated to ni. add a function for getting size of UnwindDataBlob on x86/linux. Signed-off-by: ragmani <ragmani0216@gmail.com>

ragmani · 2017-03-28T08:03:19Z

I modified it as removing personality routine and succeeded in generating to ni with readytorun option.

ragmani · 2017-03-28T08:26:31Z

I confirmed that simple ni assembly runs normally.

root@03c7d4352d95:/home/jang/DotNet/enable_x86/work# /home/jang/DotNet/enable_x86/work/ragmani/overlay.debug/corerun HelloDotNetCoreCS.dll 
Hello World
root@03c7d4352d95:/home/jang/DotNet/enable_x86/work# /home/jang/DotNet/enable_x86/work/ragmani/overlay.debug/corerun HelloDotNetCoreCS.ni.exe 
Hello World

jkotas · 2017-03-28T15:39:35Z

+    PVOID pData = GetData();
+    DWORD dwSize = GetBlobSize();
+
+    UNWIND_INFO * pUnwindInfo = (UNWIND_INFO *)pData;


This is unnecessary - pUnwindInfo is not used.

I remoted it.

jkotas · 2017-03-28T15:42:08Z

+#elif defined(_TARGET_X86_) && defined(FEATURE_PAL)
+    PTR_UNWIND_INFO pUnwindInfo(dac_cast<PTR_UNWIND_INFO>(moduleBase + RUNTIME_FUNCTION__GetUnwindInfoAddress(pRuntimeFunction)));
+
+    *pSize = ALIGN_UP(sizeof(ULONG), sizeof(DWORD));


This is not correct - it will be always 4. It needs to be the actual size like in other cases

I modified to "*pSize = ALIGN_UP(sizeof(UNWIND_INFO), sizeof(BYTE));"

Aligning to sizeof(BYTE) doesn't make sense, it would always return the first parameter of the macro unchanged (align ensures that the value returned by the macro is a multiple of the second parameter and sizeof(BYTE) is 1 and all numbers are multiple of 1)

jkotas · 2017-03-28T15:44:12Z

+
+UINT ZapUnwindData::GetAlignment()
+{
+    return sizeof(ULONG);


Can this be just sizeof(BYTE) ? The JIT case does not seem to have any extra alignment. It can be same here.

I modified to BYTE in here and GetUnwindDataBlob function.

dnfclas · 2017-03-29T06:44:11Z

@ragmani, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request.

Thanks, .NET Foundation Pull Request Bot

change Unwindinfo's size from sizeof(ULONG) to sizeof(UNWIND_INFO). change Unwindinfo's alignment from sizeof(DWORD) to sizeof(BYTE). remove unnecessary code.

janvorli · 2017-03-30T09:08:30Z

+#elif defined(_TARGET_X86_) && defined(FEATURE_PAL)
+    PTR_UNWIND_INFO pUnwindInfo(dac_cast<PTR_UNWIND_INFO>(moduleBase + RUNTIME_FUNCTION__GetUnwindInfoAddress(pRuntimeFunction)));
+
+    *pSize = ALIGN_UP(sizeof(UNWIND_INFO), sizeof(BYTE));


The first argument to ALIGN_UP is correct now, but the second should be sizeof(DWORD)

janvorli · 2017-04-03T13:40:00Z

@dotnet-bot test Ubuntu x64 Checked Build and Test please

janvorli · 2017-04-03T13:41:21Z

@dotnet-bot test OSX10.12 x64 Checked Build and Test please

parjong · 2017-04-05T05:53:27Z

@jkotas @janvorli Could you please take a look? I overhear that this PR resolves 2 readytorun test failures.

janvorli · 2017-04-05T09:19:25Z

+
+UINT ZapUnwindData::GetAlignment()
+{
+    return sizeof(BYTE);


This should be sizeof(ULONG) like on the other platforms.

This change was introduced according to the following comment by @jkotas:

Can this be just sizeof(BYTE) ? The JIT case does not seem to have any extra alignment. It can be same here.

Could you let us know the semantics of GetAlignment? x86/Linux currently emits no unwind data, and thus sizeof(BYTE) also looks fine.

Ok, I was comparing it just to the other platforms. I've missed that comment from @jkotas and he knows better than me the semantics here. So let's ignore all of my comments.

janvorli · 2017-04-05T09:22:55Z

+
+DWORD ZapUnwindData::GetSize()
+{
+    DWORD dwSize = ZapBlob::GetSize();


Please add dwSize = AlignUp(dwSize, sizeof(ULONG)); here, like we use on other platforms.

janvorli · 2017-04-05T09:26:11Z

+    PVOID pData = GetData();
+    DWORD dwSize = GetBlobSize();
+
+    pZapWriter->Write(pData, dwSize);


And finally here, I believe we should add the following after the write (again, like we do on other platforms)

DWORD dwPad = AlignmentPad(dwSize, sizeof(DWORD)); if (dwPad != 0) pZapWriter->WritePad(dwPad);

jkotas · 2017-04-05T13:39:31Z

+#elif defined(_TARGET_X86_) && defined(FEATURE_PAL)
+    PTR_UNWIND_INFO pUnwindInfo(dac_cast<PTR_UNWIND_INFO>(moduleBase + RUNTIME_FUNCTION__GetUnwindInfoAddress(pRuntimeFunction)));
+
+    *pSize = ALIGN_UP(sizeof(UNWIND_INFO), sizeof(DWORD));


This does not look right. There is no alignment when the unwind blob is getting produced, and so there should be no alignment here where it is getting consumed either.

You should see crashes during GC or exception handling with R2R images because of this bug.

Maybe this should be identical to the !WIN64EXCEPTIONS path:

PTR_VOID GetUnwindDataBlob(TADDR moduleBase, PTR_RUNTIME_FUNCTION pRuntimeFunction, /* out */ SIZE_T * pSize) { *pSize = 0; return dac_cast<PTR_VOID>(pRuntimeFunction->UnwindData + moduleBase); }

Hmm, I am confused here. The added alignment was my request, since we do align in a similar way on AMD64 too (see few lines above).

The structure of the unwind info is different on x86. There is no actual isolated unwind info blob like on x64. The unwind info is part of GCInfo on x86. So mirroring x64 is not the right strategy.

Hmm, I see, the size is used to get location of the GCInfo from the address of the unwind data blob, so returning zero size like the Windows x86 is the right thing to do here. I can see now that that's what all the callers of GetUnwindDataBlob expect for x86.

@ragmani, @parjong I am sorry for causing the confusion here.

Btw, it looks like we will need to modify the following functions to take into account the fact that there is no personality routine for x86 Linux:
NativeImageJitManager::IsFilterFunclet
ReadyToRunJitManager::IsFilterFunclet

@jkotas Should *pSize be set as 0? x86/Linux produces a unwind info that consists of FunctionLength unlike x86/Windows. (from inc/win64unwind.h).

I see - this may be just sizeof(UNWIND_INFO) for clarity then.

#10750 is submitted for cleanup.

* [x86/Linux] add three functions for _X86_TARGET_ There is no functions for _X86_TARGET_. - ZapUnwindData::GetAlignment() - DWORD ZapUnwindData::GetSize() - void ZapUnwindData::Save(ZapWriter * pZapWriter) * [x86/Linux] remove personality routine on x86/linux. remove creating personaly routine when assemblies is generated to ni. add a function for getting size of UnwindDataBlob on x86/linux. Signed-off-by: ragmani <ragmani0216@gmail.com> * [x86/Linux] correct the unclearly fixed parts. change Unwindinfo's size from sizeof(ULONG) to sizeof(UNWIND_INFO). change Unwindinfo's alignment from sizeof(DWORD) to sizeof(BYTE). remove unnecessary code. * [x86/Linux] change alignment from sizeof(BYTE) to sizeof(DWORD).

dnfclas added the cla-required label Mar 22, 2017

parjong reviewed Mar 22, 2017

View reviewed changes

parjong mentioned this pull request Mar 22, 2017

[x86/Linux] add creating the READYTORUN_HELPER_PersonalityRoutine chuck for x86/Linux. #10381

Closed

ragmani closed this Mar 24, 2017

ragmani reopened this Mar 24, 2017

dnfclas added the cla-required label Mar 24, 2017

ragmani and others added 2 commits March 27, 2017 14:06

[x86/Linux] add three functions for _X86_TARGET_

e8445dd

There is no functions for _X86_TARGET_. - ZapUnwindData::GetAlignment() - DWORD ZapUnwindData::GetSize() - void ZapUnwindData::Save(ZapWriter * pZapWriter)

[x86/Linux] remove personality routine on x86/linux.

97f888a

remove creating personaly routine when assemblies is generated to ni. add a function for getting size of UnwindDataBlob on x86/linux. Signed-off-by: ragmani <ragmani0216@gmail.com>

ragmani force-pushed the master branch from 9d04c81 to 97f888a Compare March 28, 2017 07:57

jkotas reviewed Mar 28, 2017

View reviewed changes

dnfclas added cla-signed and removed cla-required labels Mar 29, 2017

[x86/Linux] correct the unclearly fixed parts.

fdb39e2

change Unwindinfo's size from sizeof(ULONG) to sizeof(UNWIND_INFO). change Unwindinfo's alignment from sizeof(DWORD) to sizeof(BYTE). remove unnecessary code.

janvorli reviewed Mar 30, 2017

View reviewed changes

[x86/Linux] change alignment from sizeof(BYTE) to sizeof(DWORD).

ed59f53

janvorli reviewed Apr 5, 2017

View reviewed changes

janvorli merged commit c7241a9 into dotnet:master Apr 5, 2017

jkotas reviewed Apr 5, 2017

View reviewed changes

parjong mentioned this pull request Apr 5, 2017

[x86/Linux] Clean up ZapUnwindData #10750

Merged

karelz modified the milestone: 2.0.0 Aug 28, 2017

Conversation

ragmani commented Mar 22, 2017

Uh oh!

dnfclas commented Mar 22, 2017

Uh oh!

ragmani commented Mar 22, 2017

Uh oh!

parjong Mar 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parjong commented Mar 22, 2017

Uh oh!

dnfclas commented Mar 24, 2017

Uh oh!

ragmani commented Mar 28, 2017

Uh oh!

ragmani commented Mar 28, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Mar 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ragmani Mar 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfclas commented Mar 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janvorli commented Apr 3, 2017

Uh oh!

janvorli commented Apr 3, 2017

Uh oh!

parjong commented Apr 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkotas Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parjong Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

parjong Mar 22, 2017 •

edited

Loading

jkotas Mar 28, 2017 •

edited

Loading

ragmani Mar 30, 2017 •

edited

Loading

jkotas Apr 5, 2017 •

edited

Loading

parjong Apr 5, 2017 •

edited

Loading