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

Update libunwind to v1.6.2 #62092

Merged
merged 5 commits into from
Dec 15, 2021
Merged

Conversation

am11
Copy link
Member

@am11 am11 commented Nov 27, 2021

All changes upstream since v1.5-rc2 libunwind/libunwind@v1.5-rc2...v1.6.2.

Method:

  1. #!/usr/bin/env bash
    
    # backup existing libunwind
    cd src/coreclr/pal/src
    mv libunwind libunwind_old
    
    # clone new version branch
    git clone https://github.com/libunwind/libunwind --single-branch --depth 1 --branch v1.6.2
    
    # cp files and directories from the backup
    cp -r libunwind_old/{libunwind-version.txt,CMakeLists.txt,configure.cmake,config.h.in} libunwind
    cp -r libunwind_old/src/{CMakeLists.txt,oop} libunwind/src
    
    # delete .git/ dir and README.md file
    cd libunwind
    rm -rf .git README.md
    
    # make README.md symlink to README file
    ln -s README README.md
  2. Manually update src/coreclr/pal/src/libunwind/libunwind-version.txt.
  3. Manually apply changes from 1b5719c.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 27, 2021
@jkotas jkotas requested a review from janvorli November 27, 2021 00:34
@am11 am11 force-pushed the feature/vendor/update-libunwind branch 6 times, most recently from 2a74cc1 to 4858d37 Compare November 27, 2021 16:04
@am11 am11 force-pushed the feature/vendor/update-libunwind branch from 4858d37 to b3bd800 Compare November 29, 2021 23:24
@am11 am11 marked this pull request as ready for review November 30, 2021 01:31
@am11 am11 requested a review from janvorli November 30, 2021 01:52
@am11
Copy link
Member Author

am11 commented Nov 30, 2021

@janvorli, this is ready for review, could you please take a look? libunwind-version.txt has the updated info, how this copy currently differs from upstream. Thanks!

@am11 am11 force-pushed the feature/vendor/update-libunwind branch from b3bd800 to abf6037 Compare December 1, 2021 05:26
@am11 am11 changed the title Update libunwind to v1.6.0 Update libunwind to v1.6.2 Dec 1, 2021
@am11 am11 force-pushed the feature/vendor/update-libunwind branch 2 times, most recently from a097628 to cf16424 Compare December 1, 2021 05:53
@am11
Copy link
Member Author

am11 commented Dec 3, 2021

we carry this one-off API in our tree for few versions now. It would be nice to upstream it with a test+manual similar to libunwind/libunwind@a860906.

cc @mikem8361

@am11
Copy link
Member Author

am11 commented Dec 9, 2021

@janvorli, @jkotas, this is ready from my side, please let me know if there is any more feedback.

We could run the outerloop leg against this PR to make sure alpine arm64 library tests haven't regressed. Unfortunately the passing rate of the outerloop-linux pipeline is 0% in past six months:
Screenshot from 2021-12-09 13-21-42

so I'm not sure which specific tests we will be looking at to detect any regression..
it happens to be the same situation as it was the last time around #36909 (comment).

"nop\n" \
".code 32\n" \
"mov r0, #0\n" \
"stmia %[base], {r0-r14}\n" \
Copy link
Member

Choose a reason for hiding this comment

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

This is now missing my changes that added storing the floating point registers. In both the thumb and non-thumb code. For the #else branch, just adding my two lines after the stmia should be enough. For the #if branch, we would need to make sure that the PC stored by the stmia matches the new semantics of the code from the libunwind repo, that means that it points after the nop. It seems that the easiest is to store the floating point registers first and then the floating point ones. A simple approach would be to add the following before the stmia:

    "adds %[base], #64\n" \
    "vstmia %[base], {d0-d15}\n" \
    "subs %[base], #64\n" \

Copy link
Member Author

Choose a reason for hiding this comment

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

@janvorli, thanks, is it the correct patch:

diff --git a/src/coreclr/pal/src/libunwind/include/libunwind-arm.h b/src/coreclr/pal/src/libunwind/include/libunwind-arm.h
index 9108dd9d9a3..c5c2b3072ac 100644
--- a/src/coreclr/pal/src/libunwind/include/libunwind-arm.h
+++ b/src/coreclr/pal/src/libunwind/include/libunwind-arm.h
@@ -280,6 +280,9 @@ unw_tdep_context_t;
   unsigned long *unw_base = unw_ctx->regs;                              \
   __asm__ __volatile__ (                                                \
     "mov r0, #0\n"                                                      \
+    "adds %[base], #64\n"                                               \
+    "vstmia %[base], {d0-d15}\n"                                        \
+    "subs %[base], #64\n"                                               \
     "stmia %[base], {r0-r15}\n"                                         \
     "nop\n" /* align return address to value stored by stmia */         \
     : [r0] "=r" (r0) : [base] "r" (unw_base) : "memory");               \
@@ -295,6 +298,9 @@ unw_tdep_context_t;
     "nop\n"                                                             \
     ".code 32\n"                                                        \
     "mov r0, #0\n"                                                      \
+    "adds %[base], #64\n"                                               \
+    "vstmia %[base], {d0-d15}\n"                                        \
+    "subs %[base], #64\n"                                               \
     "stmia %[base], {r0-r14}\n"                                         \
     "adr r0, ret%=+1\n"                                                 \
     "str r0, [%[base], #60]\n"                                          \

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that it correct. However the thumb branch (the #else one) can be simpler, since the PC is computed and stored separately there. So we can use the following form, saving one subs.

@@ -295,6 +298,9 @@ unw_tdep_context_t;
     "nop\n"                                                             \
     ".code 32\n"                                                        \
     "mov r0, #0\n"                                                      \
     "stmia %[base], {r0-r14}\n"                                         \
     "adr r0, ret%=+1\n"                                                 \
     "str r0, [%[base], #60]\n"  
+    "adds %[base], #64\n"  
+    "vstmia %[base], {d0-d15}\n"                                        \
     "orr r0, pc, #1\n"                                                  \
     "bx r0\n" 

We could possibly get rid of the subs in the #if branch too if we made the writes backwards (instead of stmia we would use stmda), but I think it is not worth making the code less readable that way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Applied and rebased. All arm changes are in a single commit: 0ca7809.

Copy link

Choose a reason for hiding this comment

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

Even better, I think you can directly replace the nop with vstmia %[base], {d0-d15}\n without needing to adjust the base at all

Copy link
Member Author

Choose a reason for hiding this comment

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

@janvorli, can we add a test case in Pri0 category for regular PRs that will:

  • make use of arm32 fp registers and hit this path
  • fail without applying your patch

I am not sure if it is as trivial as changing this test project\s property from 1 to 0

? (as that was the project which was modified as part of the original fix 1b5719c#diff-3df4332914893fb234891c86871f559e8d91d5b604df4073c56cdc36e9217b51)

Copy link
Member

Choose a reason for hiding this comment

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

The thing is that I think we cannot enforce using non-volatile floating point registers (D8..D15) in the jitted code. Using volatile ones doesn't cause issues, as we don't need to unwind it. Also, the register needs to be modified by the native code we unwind from to cause harm. Managed code uses the windows style unwinder, so it works unrelated to this change.
We were hitting the issue in one of the tests with JIT stress enabled - the JIT stress resulted in using the D8 register. I think that might be just a coincidence though. I believe we run tests with jit stress enabled nightly or on some regular basis, so I'll check if that test still reproduces the problem so that we have some coverage.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, though you may also now need to update the register constraint to let the compiler know that you are now clobbering that register

@vtjnash good point!

Copy link
Member

Choose a reason for hiding this comment

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

@am11 I have verified that this update works with the TestConvertFromIntegral test by manually stepping through the unwinder.

Copy link
Member Author

Choose a reason for hiding this comment

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

@janvorli, thank you for all the help. :)

@ghost ghost added needs-author-action An issue or pull request that requires more info or actions from the author. and removed needs-author-action An issue or pull request that requires more info or actions from the author. labels Dec 9, 2021
@am11 am11 force-pushed the feature/vendor/update-libunwind branch from 33dcfd3 to 275c845 Compare December 9, 2021 15:01
Copy link
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

@am11 am11 force-pushed the feature/vendor/update-libunwind branch from 275c845 to e621e21 Compare December 10, 2021 00:50
@am11 am11 force-pushed the feature/vendor/update-libunwind branch from e621e21 to bb4af17 Compare December 10, 2021 01:40
Co-authored-by: Jan Vorlicek <jan.vorlicek@volny.cz>
@janvorli janvorli merged commit 74e6d01 into dotnet:main Dec 15, 2021
@am11 am11 deleted the feature/vendor/update-libunwind branch December 15, 2021 17:48
@ghost ghost locked as resolved and limited conversation to collaborators Jan 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-PAL-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants