[PATCH] D79664: [ELF] Support --pack-dyn-relocs=rel+relr

[Alexander Richardson via Phabricator via llvm-commits]

arichardson added inline comments.


================
Comment at: lld/ELF/Driver.cpp:743
-    return {false, true};
+    return {false, true, false};
+  // Note there is no plain "rel" option here since rela->rel rewriting is
+  // motivated by space savings and relr is the biggest win for that, so "rel"
+  // alone is more likely to be a typo for "relr" than an intentional selection
+  // of rela->rel without relr.
+  if (s == "rel+relr")
+    return {false, true, true};
   if (s == "android+relr")
-    return {true, true};
----------------
grimar wrote:
> aside: I wonder why we support "android+relr", but not "android,relr". The latter form looks more natural to me.
I agree. How about allowing comma-separated list where the order does not matter?


================
Comment at: lld/ELF/Driver.cpp:746
+  // alone is more likely to be a typo for "relr" than an intentional selection
+  // of rela->rel without relr.
+  if (s == "rel+relr")
----------------
I don't think it makes sense to omit an option because it might be a typo.
I doubt many people will manually pass `-Wl,--pack-dyn-relocs=`,  it will probably come from some build system default.
Those who pass it manually probably know to check the output binary to see if worked as expected.

I would quite like to see the plain rel option since it's useful on platforms that don't have RELR and also useful for LLD tests to check that we always write addends in REL format.
I noticed quite a few bugs in the RELA->REL conversion in the past (I have been working mostly with MIPS where clang emits RELA, but LLD converts them to REL). Having this conversion would allow writing tests that don't depend on MIPS.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79664