- Added WritableArmRelocation and ArmRelocation Structs
- Encode/Decode funcs for B/BL A1 and BLX A2 encodings
- Add ARM helper functions, consistent with the existing Thumb helper functions
- Add Test for ELF::R_ARM_CALL
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Looks great, thanks! I think at some point we should add a .thumb section with .thumb_func functions to our tests, so we get coverage for the code that handles ARM-Thumb transitions. However, I guess we need ARM stubs for it first?
llvm/lib/ExecutionEngine/JITLink/aarch32.cpp | ||
---|---|---|
295 | Oh right, this has to go away! | |
424 | I think we don't need to worry about alignment here, because Thumb requirements are lower than ARM. Ideally we'd have test coverage though :) |
NFC
- Add descr to Arm_Call enum.
- Remove alignment related comments.
- Standardize comment marker as '#'
Can we align the test with the (now) existing ELF_static_arm_reloc.s please? The rest should be ready to land I think.
llvm/lib/ExecutionEngine/JITLink/aarch32.cpp | ||
---|---|---|
21 | You include it in the header already, so it isn't necessary here. |
Thanks for the updates! Please have a look at one last detail in the test.
llvm/test/ExecutionEngine/JITLink/AArch32/ELF_static_arm_reloc.s | ||
---|---|---|
17 | This is correct and works, but the attentive reader might wonder where the -4 is coming from. The branch offset in Thumb state is equivalent to the size of the branch instruction, i.e. next_pc does a +4 and thus we don't need the -4 in the Thumb test here. Surprisingly, the branch offset in ARM state is 8 and not 4 as next_pc tells us. That's because the branch instruction increments the PC register twice since it runs an implicit prefetch instruction (seems to be a ARM legacy thing that the linker has to know that). I guess ideally we could write next_pc(next_pc(call_site)) but the checker cannot evaluate that expression. Thus, I wonder if it would be better to drop the next_pc here, write out the branch offset explicitly and add a comment like this. What do you think? |
llvm/test/ExecutionEngine/JITLink/AArch32/ELF_static_arm_reloc.s | ||
---|---|---|
17 | The magic number 4 doesn't really look good and it makes sense to show the condition explicitly. Thanks for the explanation of why the offset is present! |
@Eymay and @sgraenitz this commit seems to be causing build failures on Windows build bots, can you take a look and revert if you need time to investigate?
C:\bin\ccache.exe C:\PROGRA~2\MICROS~1\2019\BUILDT~1\VC\Tools\MSVC\1429~1.301\bin\HostX64\x64\cl.exe /nologo /TP -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_GLIBCXX_ASSERTIONS -D_HAS_EXCEPTIONS=0 -D_LIBCPP_ENABLE_HARDENED_MODE -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Iunittests\ExecutionEngine\JITLink -IZ:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink -Iinclude -IZ:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\include -IZ:\b\llvm-clang-x86_64-sie-win\llvm-project\third-party\unittest\googletest\include -IZ:\b\llvm-clang-x86_64-sie-win\llvm-project\third-party\unittest\googlemock\include /DWIN32 /D_WINDOWS /Zc:inline /Zc:preprocessor /Zc:__cplusplus /Oi /bigobj /permissive- /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd5105 -wd4324 -w14062 -we4238 /Gw /O2 /Ob2 -MD /EHs-c- /GR- -UNDEBUG -std:c++17 /showIncludes /Founittests\ExecutionEngine\JITLink\CMakeFiles\JITLinkTests.dir\AArch32Tests.cpp.obj /Fdunittests\ExecutionEngine\JITLink\CMakeFiles\JITLinkTests.dir\ /FS -c Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(187): error C3493: 'ImmMask' cannot be implicitly captured because no default capture mode has been specified Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(194): error C2064: term does not evaluate to a function taking 2 arguments Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(194): error C2660: 'testing::internal::EqHelper::Compare': function does not take 3 arguments Z:\b\llvm-clang-x86_64-sie-win\llvm-project\third-party\unittest\googletest\include\gtest/gtest.h(1562): note: see declaration of 'testing::internal::EqHelper::Compare' Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(194): error C2737: 'gtest_ar': const object must be initialized Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(195): error C2064: term does not evaluate to a function taking 2 arguments Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(195): error C2660: 'testing::internal::EqHelper::Compare': function does not take 3 arguments Z:\b\llvm-clang-x86_64-sie-win\llvm-project\third-party\unittest\googletest\include\gtest/gtest.h(1562): note: see declaration of 'testing::internal::EqHelper::Compare' Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(195): error C2737: 'gtest_ar': const object must be initialized Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(196): error C2064: term does not evaluate to a function taking 2 arguments Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(196): error C2660: 'testing::internal::EqHelper::Compare': function does not take 3 arguments Z:\b\llvm-clang-x86_64-sie-win\llvm-project\third-party\unittest\googletest\include\gtest/gtest.h(1562): note: see declaration of 'testing::internal::EqHelper::Compare' Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(196): error C2737: 'gtest_ar': const object must be initialized Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(197): error C2064: term does not evaluate to a function taking 2 arguments Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(197): error C2660: 'testing::internal::EqHelper::Compare': function does not take 3 arguments Z:\b\llvm-clang-x86_64-sie-win\llvm-project\third-party\unittest\googletest\include\gtest/gtest.h(1562): note: see declaration of 'testing::internal::EqHelper::Compare' Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(197): error C2737: 'gtest_ar': const object must be initialized Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(198): error C2064: term does not evaluate to a function taking 2 arguments Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(198): error C2660: 'testing::internal::CmpHelperNE': function does not take 3 arguments Z:\b\llvm-clang-x86_64-sie-win\llvm-project\third-party\unittest\googletest\include\gtest/gtest.h(1620): note: see declaration of 'testing::internal::CmpHelperNE' Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(198): error C2737: 'gtest_ar': const object must be initialized Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(199): error C2064: term does not evaluate to a function taking 2 arguments Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(199): error C2660: 'testing::internal::CmpHelperNE': function does not take 3 arguments Z:\b\llvm-clang-x86_64-sie-win\llvm-project\third-party\unittest\googletest\include\gtest/gtest.h(1620): note: see declaration of 'testing::internal::CmpHelperNE' Z:\b\llvm-clang-x86_64-sie-win\llvm-project\llvm\unittests\ExecutionEngine\JITLink\AArch32Tests.cpp(199): error C2737: 'gtest_ar': const object must be initialized
Some affected build bots:
I can confirm this seems to be holding up presubmit testing on windows builds.
https://buildkite.com/llvm-project/github-pull-requests/builds/1789#018a8faa-2d4f-4850-93a2-13fa03d20dac
We've experienced a similar issue and added a fix in an upcoming patch Implement ELF::R_ARM_MOVT_ABS and R_ARM_MOVW_ABS_NC. I don't have commit access so you may revert it until the new patch arrives and fixes it.
Is there an estimate when you might be able to commit the upcoming patch with a fix? A quick revert attempt shows conflicts meaning I would have to revert some further downline patches as well.
You include it in the header already, so it isn't necessary here.