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

[George Rimar via Phabricator via llvm-commits]

grimar added a comment.

In D79664#2028360 <https://reviews.llvm.org/D79664#2028360>, @mcgrathr wrote:

> In D79664#2028262 <https://reviews.llvm.org/D79664#2028262>, @MaskRay wrote:
>
> > I haven't thought much on this yet. First impression: why isn't the RELA->REL toggle a new option? It does not seem to fit with the rest of --pack-dyn-relocs which selects Android packed dynamic relocations or RELR or both.
>
>
> --pack-dyn-relocs seems exactly appropriate to me since it's about packing the dynamic relocs.  It does RELA->REL conversion just as the existing option does REL(A)->RELR conversion.  But I'm not concerned with the switch syntax.  I'm fine with whatever syntax people prefer.  (For example, I don't at all think it's important to disallow the RELA->REL conversion without RELR support, I just don't care about that mode at all.)  I probably wouldn't have added --pack-dyn-relocs with its string syntax in the first place, but instead just added separate `-z android-relocs` and `-z relr` switches so the new one would be `-z rel` or something.  But given --pack-dyn-relocs exists, it seems like the natural place for this to me now.


I am inclined to agree. I've added a few suggestions about the code.



================
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};
----------------
aside: I wonder why we support "android+relr", but not "android,relr". The latter form looks more natural to me.


================
Comment at: lld/ELF/Driver.cpp:755
+  return {false, false, false};
 }
 
----------------
Its a bit hard to read a tuple of 3 `bool`s probably? Perhaps I'd do the same what we do for `-hash-style`.
(See "Parse -hash-style={sysv,gnu,both}.") below.

I.e. I'd:
1) Inline this method.
2) Set `config->androidPackDynRelocs`, `config->relrPackDynRelocs`,
           `config->relPackDynRelocs` to `false` by default.

And do something similar to what `-hash-style` does:

```
  // Parse -hash-style={sysv,gnu,both}.
  if (auto *arg = args.getLastArg(OPT_pack_dyn_relocs)) {
    StringRef s = arg->getValue();
    if (s.startsWith("android")) {
      config->androidPackDynRelocs = true;
      // Check "+relr" ..
    } else if ...
    ....
  }
```




================
Comment at: lld/ELF/Driver.cpp:1222
   //
-  // You cannot choose which one, Rel or Rela, you want to use. Instead each
-  // ABI defines which one you need to use. The following expression expresses
-  // that.
+  // The psABI for each processor specifies the canonical one to use.  The
+  // following expression expresses that.  But actual compatibility depends on
----------------
This comment have double spaces after full stops.


================
Comment at: lld/ELF/Driver.cpp:1235
+  if (config->relPackDynRelocs)
+    config->isRela = false;
 
----------------
I believe usually we assign config variables once. So it could be:

```
config->isRela = !config->relPackDynRelocs && ...
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79664