[PATCH] D127220: [BOLT][AArch64] Preserve in text object alignment

[Rafael Auler via Phabricator via llvm-commits]

rafauler added a comment.

Thanks @yota9 for working on this! I have some questions below.



================
Comment at: bolt/include/bolt/Core/BinaryFunction.h:1104
+  // Return original alignment value of the function
+  uint16_t getInputAlignment() const { return 1 << (ffsll(getAddress()) - 1); }
+
----------------
In general in BOLT we have no way of recovering the correct original alignment. I'm afraid having a function that returns this information might be misleading to users of this API, I'm more comfortable with a function named "guessInputAlignment" than one that says "getInputAlignment".


================
Comment at: bolt/lib/Passes/Aligner.cpp:172-173
     if (!BF.size() && BF.hasIslandsInfo()) {
-      const uint16_t Alignment = BF.getConstantIslandAlignment();
+      const uint16_t Alignment = BF.getInputAlignment();
+      BF.setConstantIslandAlignment(Alignment);
       if (BF.getAlignment() < Alignment)
----------------
I don't completely follow this part and perhaps we can clarify: why are we aligning a constant island against the presumed alignment of its parent function, and not against the alignment of the constant island itself in the input?

Another thing that could be an issue here is if the function happens to be at an arbitrary round address (such as 0x100000) and now we have to emit the island with MBs worth of alignment because we have no clue what is the correct alignment. That's bound to happen if we are processing the very first function in the .text section and if it has a constant island, since that first function will probably be aligned to a page boundary, which may push the constant island beyond the reach of some instructions. That's why in general I think that's a fragile strategy, but if we have to do it and if we are resorting to guessing the correct alignment, maybe we should put a cap on it? But before proceeding with this diff, I have one alternative suggestion below.

I read the offending code in openssl. Can we try -skip-funcs=problematic_function_name and just skip supporting it (I'm not sure if that works with AArch64, though). I think BOLT is not in a position of supporting arbitrary assembly code, and code that makes assumptions on the layout of functions is bound to break. This hits AArch64 more strongly because it is okay to have data in code for AArch64, and some programmers like making assumptions on the layout of the data. That's why it's easier for us to provide support for languages such as C++, which has a standard that says that doing pointer arithmetic with function/object pointers is undefined behavior.




Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D127220/new/

https://reviews.llvm.org/D127220