[PATCH] D37585: Add ARM backend support for pagerando

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

I'm mostly a linker person so I'm not best qualified to review the backend changes. I've tried to take a look from an overall toolchain perspective.

How does pagerando handle the Arm exception tables? I can't immediately see how they will work for binned functions. My understanding is that for each executable Section the compiler will output a .ARM.exidx section with a SHF_LINK_ORDER dependency on the executable Section. The format of the .ARM.exidx section is one entry per function | R_ARM_PREL31 offset to function (usually expressed as Section Symbol + offset) | inline unwinding instructions or R_ARM_PREL31 offset to .ARM.extab |. At link time after SHF_LINK_ORDER has been performed we are left with a single .ARM.exidx section containing a table of pc-relative offsets to functions that can be binary searched by the unwinder to find out how to unwind the stack when __cxa_throw is called. We obviously can't use pc-relative offsets to the binned functions (unless we are willing to make the .ARM.exidx section read/write and have the loader update the table). The pc-relative offset could be made to the wrapper functions (no .ARM.exidx section for the Section containing the binned functions, make sure each section containing wrappers has a .ARM.exidx section), but given that the call to cxa_throw would come from the binned function and not the wrapper the unwinder wouldn't be able to find the pc-value at the cxa_throw call site in the table unless the wrapper always tail-called.

My apologies if I'm missing something obvious. Can you add a test that involves exceptions if you have this covered already?



================
Comment at: lib/Target/ARM/ARMAsmPrinter.cpp:891
+    //
+    // Note: This requires LTO.
+    auto *F = cast<Function>(cast<ARMConstantPoolConstant>(ACPV)->getGV());
----------------
What happens if LTO is not used? Is it just that only the current module gets put into bins and the rest isn't which is sub-optimal but correct or is it fatal? If it is fatal is there any way of asserting that LTO is required, if not then can the comment be expanded?


================
Comment at: lib/Target/ARM/ARMConstantPoolValue.h:224
   ARMConstantPoolSymbol(LLVMContext &C, StringRef s, unsigned id,
-                        unsigned char PCAdj, ARMCP::ARMCPModifier Modifier,
+                        unsigned char PCAdj, ARMCP::ARMCPModifier Mvodifier,
                         bool AddCurrentAddress);
----------------
Typo "Mvodifier" should be "Modifier"


================
Comment at: lib/Target/ARM/ARMFastISel.cpp:2969
+  ARMConstantPoolValue *CPV;
+  if (Subtarget->isPIP()) {
+    CPV = ARMConstantPoolConstant::Create(GV, ARMCP::GOTOFF);
----------------
style nit, I think that there is only one statement so the braces aren't needed.


================
Comment at: lib/Target/ARM/ARMFastISel.cpp:2994
+  if (Subtarget->isPIP()) {
+    // Add POT[0] (GOT address)
+    unsigned POTReg = MF->addLiveIn(TLI.getPOTBaseRegister(), &ARM::rGPRRegClass);
----------------
Would "Add the GOT address from POT[0]" be a bit clearer?


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:2050
+      ARMConstantPoolValue *CPV;
+      if (Subtarget->isPIP())
+        CPV = ARMConstantPoolConstant::Create(GV, ARMCP::GOTOFF);
----------------
Given the assert above, could this be true?


https://reviews.llvm.org/D37585