This is an archive of the discontinued LLVM Phabricator instance.

[JITLink][AArch32] Implement ELF::R_ARM_CALL relocation
ClosedPublic

Authored by Eymay on Aug 9 2023, 11:39 AM.

Details

Summary
  • 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

Diff Detail

Unit TestsFailed

Event Timeline

Eymay created this revision.Aug 9 2023, 11:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2023, 11:39 AM
Eymay requested review of this revision.Aug 9 2023, 11:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2023, 11:39 AM

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
294

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 :)

Eymay added a comment.Aug 14 2023, 2:34 AM

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?

Great idea, I agree an arm_thumb_interworking test is necessary. Without stubs for Arm, we should be able to test bl thumb_func as long as it is in range.
Another thing we need, for this test to work, is a TargetFlag dependent triple like @lhames mentioned on discourse.

Eymay updated this revision to Diff 550746.Aug 16 2023, 7:25 AM

NFC

  • Add descr to Arm_Call enum.
  • Remove alignment related comments.
  • Standardize comment marker as '#'
Eymay marked an inline comment as done.Aug 16 2023, 7:26 AM

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.

Eymay updated this revision to Diff 556546.Sep 12 2023, 4:13 AM

Updated test regarding ELF_static_thumb_reloc.s and squashed commits

Eymay marked an inline comment as done.Sep 12 2023, 4:14 AM

Can we align the test with the (now) existing ELF_static_arm_reloc.s please? The rest should be ready to land I think.

Sure, updated the test.

Eymay updated this revision to Diff 556652.Sep 13 2023, 4:36 AM

(NFC) Updated enc/decode description comment bits with precise number of 0's

Eymay updated this revision to Diff 556653.Sep 13 2023, 4:49 AM

(NFC) Removed leading zeros from immediates in comments

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?

Eymay added inline comments.Sep 13 2023, 7:08 AM
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 updated this revision to Diff 556662.Sep 13 2023, 7:12 AM

Update jitlink-check test to indicate the offset explicitly.

Eymay marked an inline comment as done.Sep 13 2023, 7:12 AM
Eymay updated this revision to Diff 556663.Sep 13 2023, 7:23 AM

update author mail

sgraenitz accepted this revision.Sep 13 2023, 7:30 AM

Thanks! Let me double-check the tests on my machine and land this for you.

This revision is now accepted and ready to land.Sep 13 2023, 7:30 AM
This revision was landed with ongoing or failed builds.Sep 13 2023, 8:17 AM
This revision was automatically updated to reflect the committed changes.
dyung added a subscriber: dyung.Sep 13 2023, 12:45 PM

@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:

Eymay added a comment.Sep 13 2023, 2:10 PM

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.

dyung added a comment.Sep 13 2023, 2:25 PM

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.

I can fix it now