[PATCH] D126177: [BOLT] [AArch64] Handle data markers spanning multiple functions

[Rafael Auler via Phabricator via llvm-commits]

rafauler added a comment.

Thanks, @treapster!

Sorry, got confused, you're absolutely right on the comparison function requirement.

Regarding the check, I would prefer to preserve the check on AArch64 to reduce the amount of work we do for X86 symbols. I think we can keep these functions in BinaryContext. We wouldn't be able to push this to SymbolRef anyway because this lives in another llvm lib, and we would need to better justify its usage outside just BOLT. I just felt that you put "isDataMarker" and "isCodeMarker" there in case you would need these functions (as we had them before), but then you wrote the solution in a way that doesn't use them, so they look like dead code to me and should be removed (leaving only getMarkerType and isMarker).

I gave another thought on this and I agree with you on the other topics (efficiency, etc), so this is good to go from my perspective, pending fixing the testcase and removing isCodeMarker/isDataMarker. Thanks for working on this and submitting the patch!



================
Comment at: bolt/test/AArch64/unmarked-data.s:3
+
+// RUN: llvm-mc -filetype=obj %s -o %t.o
+// RUN: ld.lld %t.o -o %t.exe -q
----------------
I guess you have to include -triple aarch64-unknown-linux as an argument to llvm-mc, otherwise it will fail in non-aarch64 machines


================
Comment at: bolt/test/AArch64/unmarked-data.s:5-9
+// COM: We make sure that symbol second is not marked as data by compiler.
+// COM: We find it's address, then find all symbols at that address and check there is no $d marker
+// RUN: out=`llvm-readelf -Ws %t.exe | grep second`; str=($(echo "$out" | tr '\t' '\n')); addr=${str[1]}; \
+// RUN: llvm-readelf -Ws %t.exe | grep $addr | FileCheck %s
+// CHECK-NOT: $d
----------------
While I do appreciate unix style command line processing, I feel we should be careful with this in case we run this in an environment without shell (e.g. Windows). I'm not an expert in Windows support and we currently do not have a Windows buildbot for BOLT, but I feel this would probably fail in Visual Studio.

We should probably remove these lines too as, from a test perspective, this is enforcing that llvm-mc and ld.lld always behave in that specific way regarding emission of "$d" symbols. If somebody tries to change that, this test will break, even though they are not touching BOLT. Here we should limit ourselves to test BOLT.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D126177