[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};
----------------
MaskRay wrote:
> grimar wrote:
> > arichardson wrote:
> > > 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?
> > It would look better to me.
> Note, in compiler drivers (GCC+clang), -Wl, does not have escapes for commas, so commas in the linker generally does not work well.
That is a good very good reason not to use `,`.
Maybe allowing the option multiple times is the best solution. Or if that is too verbose, `+` or `:` as a separator should be fine. 
I would also prefer if we didn't require a fixed order: why should `rel+relr` be accepted but not `relr+rel`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79664