[llvm] [BOLT] Support instrumentation hook via DT_FINI_ARRAY (PR #67348)

[Job Noorman via llvm-commits]

================
@@ -1276,6 +1282,68 @@ void RewriteInstance::discoverFileObjects() {
   registerFragments();
 }
 
+Error RewriteInstance::discoverRtFiniAddress() {
+  // If FiniFunctionAddress is already set, we got if from DT_FINI. We use
+  // DT_FINI instead of DT_FINI_ARRAY if it's available.
+  if (BC->FiniFunctionAddress)
+    return Error::success();
+
+  if (!BC->FiniArrayAddress || !BC->FiniArraySize) {
+    return createStringError(
+        std::errc::not_supported,
+        "Instrumentation needs either DT_FINI or DT_FINI_ARRAY");
+  }
+
+  if (*BC->FiniArraySize < BC->AsmInfo->getCodePointerSize()) {
+    return createStringError(std::errc::not_supported,
+                             "Need at least 1 DT_FINI_ARRAY slot");
+  }
+
+  ErrorOr<BinarySection &> FiniArraySection =
+      BC->getSectionForAddress(*BC->FiniArrayAddress);
+  if (auto EC = FiniArraySection.getError())
+    return errorCodeToError(EC);
+
+  BC->FiniArraySection = &*FiniArraySection;
+
+  if (const Relocation *Reloc = FiniArraySection->getDynamicRelocationAt(0)) {
+    BC->FiniFunctionAddress = Reloc->Addend;
+    return Error::success();
+  }
+
+  if (const Relocation *Reloc = FiniArraySection->getRelocationAt(0)) {
+    BC->FiniFunctionAddress = Reloc->Value;
+    return Error::success();
+  }
+
+  return createStringError(std::errc::not_supported,
+                           "No relocation for first DT_FINI_ARRAY slot");
+}
+
+void RewriteInstance::updateRtFiniReloc() {
+  if (!BC->FiniArraySection)
----------------
mtvec wrote:

> I didn't notice it before to be honest, but the problem with your current change that after you set the FiniFunctionAddress you can't distinguish between the case where DT_FINI was originally in the binary and you don't have to patch DT_FINI_ARRAY and your case where you should. Correct me if I'm wrong.

Ugh, nothing to correct, you're right of course :) I completely missed this.

I fixed this similar to what you suggest by introducing a new `FiniAddress` variable. I think this makes things more consistent:
- `DT_FINI` -> `FiniAddress`;
- `DT_FINI_ARRAY` -> `FiniArrayAddress`;
- `DT_FINI_ARRAYSZ` -> `FiniArraySize`.

The original `FiniFunctionAddress` is kept as-is and is set either to `FiniAddress` (if it has a value) or to one of the relocs (if `FiniAddress` has no value). So the way to distinguish between using `DT_FINI` or `DT_FINI_ARRAY` is by checking if `FiniAddress` has a value.

Do you think this addresses the issue? In particular, I wondered about this:

> So you need to check that FiniFunctionAddress == Reloc->Value of the first dynamic slot to continue patching at least.

I believe this is not necessary because if `FiniAddress` does not have a value, the first reloc *must* be the one we want to patch (as this was checked in `discoverRtFiniAddress`).

https://github.com/llvm/llvm-project/pull/67348