[PATCH] D124792: [ifs] Add --strip-size flag

[Alex Brachet via Phabricator via llvm-commits]

abrachet marked 3 inline comments as done.
abrachet added inline comments.


================
Comment at: llvm/docs/CommandGuide/llvm-ifs.rst:198
+.. option:: --strip-size
+ This removes the size from the output ifs and defaults the size of global
+ objects to 1 in stub output. This is useful for shared objects that don't
----------------
jhenderson wrote:
> It would be nice of this wording was made more consistent with other options in this file.
Did my best, feel free to suggest something else.


================
Comment at: llvm/lib/InterfaceStub/ELFObjHandler.cpp:222
+      if (Size == std::numeric_limits<uint64_t>::max())
+        Size = 1;
+      DynSym.Content.add(DynStr.Content.getOffset(Sym.Name), Size, Bind,
----------------
jhenderson wrote:
> abrachet wrote:
> > jhenderson wrote:
> > > Why is `Size` set to 1 and not 0, which would seem the more natural value?
> > Linkers would get upset. https://github.com/llvm/llvm-project/blob/main/lld/ELF/Relocations.cpp#L355-L358
> But giving a value of 1 means the relocation becomes invalid? It seems to me if you care about linker behaviour around copy relocations, you can't change their size at all.
You're right, I like the idea of failing fast when stripping size and using those stubs with non-pic


================
Comment at: llvm/lib/InterfaceStub/IFSHandler.cpp:126
     } else if (Symbol.Type == IFSSymbolType::Func) {
       Symbol.Size = 0;
     } else {
----------------
jhenderson wrote:
> This is a very weird thing to do in a function about writing YAML. Do you need to do it, and if so, do you need to do it here specifically?
Indeed, I think I removed this in the first diff, but accidentally added it back in the second. Thanks


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

https://reviews.llvm.org/D124792