[RyuJIT/ARM32] Support for CFI unwind info by sergign60 · Pull Request #14447 · dotnet/coreclr

sergign60 · 2017-10-12T11:55:07Z

No description provided.

sergign60 · 2017-10-16T15:54:58Z

@jkotas @BruceForstall @dotnet/jit-contrib
CC: @Dmitri-Botcharnikov @BredPet @alpencolt
please review

sergign60 · 2017-10-18T07:34:30Z

dotnet/llilc#1011

sergign60 · 2017-10-18T07:34:48Z

#2868

sergign60 · 2017-10-18T07:35:42Z

dotnet/corert#752

sergign60 · 2017-10-18T13:11:17Z

dotnet/corert#4755

sergign60 · 2017-10-18T13:13:24Z

dotnet/corert#4626

BruceForstall · 2017-10-19T01:52:36Z

src/jit/compiler.h

+
 #endif // _TARGET_ARMARCH_

+#if defined(UNIX_AMD64_ABI) || defined(_TARGET_UNIX_)


I think it's sufficient to just use:

#if defined(_TARGET_UNIX_)

since if UNIX_AMD64_ABI is defined, then _TARGET_UNIX_ will also be defined.

This comment applies to other locations as well.

BruceForstall · 2017-10-19T01:55:03Z

src/jit/compiler.h

 #endif // _TARGET_ARM_

+#if defined(UNIX_AMD64_ABI) || defined(_TARGET_UNIX_)
+    int Compiler::mapRegNumToDwarfReg(regNumber reg);


You don't need the Compiler:: here, do you?

BruceForstall · 2017-10-19T01:55:41Z

src/jit/compiler.h

+    void unwindBegPrologCFI();
+    void unwindAllocStackCFI(unsigned size);
+    void unwindSetFrameRegCFI(regNumber reg, unsigned offset);
+#if DEBUG


This should be #ifdef DEBUG

BruceForstall · 2017-10-19T02:00:18Z

src/jit/unwindarm64.cpp

+{
+    int dwarfReg = DWARF_REG_ILLEGAL;
+
+    return dwarfReg;


Is this unimplemented? Should it be:

NYI("CFI codes");

?

BruceForstall · 2017-10-19T02:00:46Z

src/jit/unwindx86.cpp

+{
+    int dwarfReg = DWARF_REG_ILLEGAL;
+
+    return dwarfReg;


Is this unimplemented? Should it be:

NYI("CFI codes");

?

BruceForstall · 2017-10-19T02:03:28Z

src/jit/unwindarm.cpp


+#if defined(_TARGET_UNIX_)
+
+#if defined(_TARGET_ARM_)


How about reducing the two #if to one line?

BruceForstall · 2017-10-19T02:10:03Z

src/jit/unwindarm.cpp

+            {
+                unwindPushCFI(regNum);
+            }
+        }


This loop looks the same as the one for unwindPushMaskInt(). How about creating a function (unwindPushCFIMask()?)

Note that you could optimize the float case by starting at the first float register; currently it's iterating through all the integer registers just to get to the first float reg.

(You could also use the BitScanForward/BitScanForward64 intrinsics, if you're really worried about perf)

@BruceForstall Done

BruceForstall · 2017-10-19T02:15:58Z

src/jit/unwindarm.cpp


+#if defined(_TARGET_UNIX_)
+    if (generateCFIUnwindCodes())
+    {


Can you extract this out to a unwindEmitFuncCFI?

@BruceForstall Done

sergign60 · 2017-10-19T12:27:37Z

@BruceForstall Thanks for your notes. I think that I've satisfied all of them

BruceForstall · 2017-10-19T16:28:07Z

Ignoring CentOS; it's failing all jobs.

briansull · 2017-10-19T22:48:36Z

src/jit/unwind.cpp

+    eeAllocUnwindInfo((BYTE*)pHotCode, nullptr /* pColdCode */, startOffset, endOffset, unwindCodeBytes, pUnwindBlock,
+                      (CorJitFuncKind)func->funKind);
+
+    if (pColdCode != NULL)


This NULL needs to be changed to nullptr for clang-tidy to pass

Added PR #14602

@briansull @benaadams Thanks!

dnfclas added the cla-already-signed label Oct 12, 2017

sergign60 changed the title ~~[RyuJIT/ARM32] Support for CFI unwind info~~ [RyuJIT/ARM32][WIP][DO NOT MERGE] Support for CFI unwind info Oct 12, 2017

sergign60 force-pushed the arm_build branch 8 times, most recently from d1fb8da to 031662e Compare October 16, 2017 14:42

sergign60 changed the title ~~[RyuJIT/ARM32][WIP][DO NOT MERGE] Support for CFI unwind info~~ [RyuJIT/ARM32] Support for CFI unwind info Oct 16, 2017

sergign60 force-pushed the arm_build branch from e136f2e to af3ce52 Compare October 17, 2017 11:56

jkotas requested a review from sandreenko October 18, 2017 12:47

sergign60 mentioned this pull request Oct 18, 2017

[RyuJIT/ARM32] Support for CFI unwind info dotnet/corert#4755

Merged

sergign60 mentioned this pull request Oct 18, 2017

[RyuJIT/ARM32] GC fails with FindProcInfo dotnet/corert#4626

Closed

BruceForstall self-requested a review October 19, 2017 01:03

BruceForstall suggested changes Oct 19, 2017

View reviewed changes

sergign60 force-pushed the arm_build branch from 854079d to 4165bce Compare October 19, 2017 12:22

sergign60 added 2 commits October 19, 2017 16:47

[RyuJIT/ARM32] Support for CFI unwind info

092e572

Deleted redundant using UNIX_AMD64_ABI

199d67c

sergign60 force-pushed the arm_build branch from b0f9f30 to 199d67c Compare October 19, 2017 13:48

BruceForstall approved these changes Oct 19, 2017

View reviewed changes

BruceForstall merged commit 3544094 into dotnet:master Oct 19, 2017

briansull mentioned this pull request Oct 19, 2017

Merge changes from TFS #14597

Merged

briansull reviewed Oct 19, 2017

View reviewed changes

benaadams mentioned this pull request Oct 20, 2017

Fix jit fromatting #14602

Merged

sergign60 deleted the arm_build branch October 20, 2017 09:42


		#endif // _TARGET_ARMARCH_

		#if defined(UNIX_AMD64_ABI) \|\| defined(_TARGET_UNIX_)

Conversation

sergign60 commented Oct 12, 2017

Uh oh!

sergign60 commented Oct 16, 2017

Uh oh!

sergign60 commented Oct 18, 2017

Uh oh!

sergign60 commented Oct 18, 2017

Uh oh!

sergign60 commented Oct 18, 2017

Uh oh!

sergign60 commented Oct 18, 2017

Uh oh!

sergign60 commented Oct 18, 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!

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!

sergign60 Oct 19, 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!

sergign60 commented Oct 19, 2017

Uh oh!

BruceForstall commented Oct 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergign60 Oct 20, 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sergign60 Oct 19, 2017 •

edited

Loading

sergign60 Oct 20, 2017 •

edited

Loading