[ifs] Add --strip-size flag

[abrachet]

Summary

st_size may not be of importance to the abi if you are not using copy relocations. This is helpful when you want to check the abi of a shared object both when instrumented and not because asan will increase the size of objects to include the redzone.

[jhenderson]

I'd like to see an llvm-ifs Command Guide document wtitten sooner rather than later, since all llvm tools should have one. Please look at doing this soon.

llvm/lib/InterfaceStub/ELFObjHandler.cpp

222

Why is Size set to 1 and not 0, which would seem the more natural value?

llvm/test/tools/llvm-ifs/strip-size.test

8

llvm/tools/llvm-ifs/llvm-ifs.cpp

441

Could you make Sym.Size Optional instead? That would seem like a cleaner interface to me.

[abrachet]

In D124792#3487528, @jhenderson wrote:

I'd like to see an llvm-ifs Command Guide document wtitten sooner rather than later, since all llvm tools should have one. Please look at doing this soon.

Looks like @haowei already created it. Added documentation for --strip-size

llvm/lib/InterfaceStub/ELFObjHandler.cpp

222

Linkers would get upset. https://github.com/llvm/llvm-project/blob/main/lld/ELF/Relocations.cpp#L355-L358

llvm/tools/llvm-ifs/llvm-ifs.cpp

441

Sure

[jhenderson]

llvm/docs/CommandGuide/llvm-ifs.rst

198

It would be nice of this wording was made more consistent with other options in this file.

llvm/lib/InterfaceStub/ELFObjHandler.cpp

222

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.

llvm/lib/InterfaceStub/IFSHandler.cpp

120–121

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?

[abrachet]

llvm/docs/CommandGuide/llvm-ifs.rst

198

Did my best, feel free to suggest something else.

llvm/lib/InterfaceStub/ELFObjHandler.cpp

222

You're right, I like the idea of failing fast when stripping size and using those stubs with non-pic

llvm/lib/InterfaceStub/IFSHandler.cpp

120–121

Indeed, I think I removed this in the first diff, but accidentally added it back in the second. Thanks

[jhenderson]

Three nits, but otherwise LGTM, with the caveat that I'm not a user or regular developer of this tool, so there might be some cases I haven't considered. If there's someone else available to sign off too, that would be good.

llvm/docs/CommandGuide/llvm-ifs.rst

201–202

Not sure if the "and" should be "or".

llvm/lib/InterfaceStub/IFSHandler.cpp

121–126

llvm/tools/llvm-ifs/llvm-ifs.cpp

440

I'd probably explicitly write out the type of Sym, since it is not obvious from this location (see the coding standards on use of auto).

[haowei]

Thanks for working on this.

[abrachet]

Fix unittests

llvm/docs/CommandGuide/llvm-ifs.rst

  This flag strips the ``Target`` field from the IFS file so it can be overridden
  later.
 
 .. option:: --write-if-changed
 
  When this flag is set, llvm-ifs will only write the output file if it does not
  already exist or the content will be different from the existing file.
 
+.. option:: --strip-size
+
+ When this flag is set, llvm-ifs will remove the size field from the output ifs
+ file. This is useful for shared objects that only intend to be linked against
+ position independent code which doesn't need copy relocations, or where the size
+ of an object is not a useful part of the abi to track.
+
 EXIT STATUS
 -----------
 
 If :program:`llvm-ifs` succeeds, it will exit with 0. Otherwise, if an
 error occurs, it will exit with a non-zero value.

llvm/include/llvm/InterfaceStub/IFSStub.h

enum class IFSBitWidthType {
   // Bit width info is 1 bytes, 256 is safely out of range.
   Unknown = 256,
 };
 
 struct IFSSymbol {
   IFSSymbol() = default;
   explicit IFSSymbol(std::string SymbolName) : Name(std::move(SymbolName)) {}
   std::string Name;
-  uint64_t Size;
+  Optional<uint64_t> Size;
   IFSSymbolType Type;
   bool Undefined;
   bool Weak;
   Optional<std::string> Warning;
   bool operator<(const IFSSymbol &RHS) const { return Name < RHS.Name; }
 };
 
 struct IFSTarget {

llvm/lib/InterfaceStub/ELFObjHandler.cpp

explicit ELFStubBuilder(const IFSStub &Stub) {
 
     // Populate dynamic symbol table.
     for (const IFSSymbol &Sym : Stub.Symbols) {
       uint8_t Bind = Sym.Weak ? STB_WEAK : STB_GLOBAL;
       // For non-undefined symbols, value of the shndx is not relevant at link
       // time as long as it is not SHN_UNDEF. Set shndx to 1, which
       // points to ".dynsym".
       uint16_t Shndx = Sym.Undefined ? SHN_UNDEF : 1;
-      DynSym.Content.add(DynStr.Content.getOffset(Sym.Name), Sym.Size, Bind,
+      uint64_t Size = Sym.Size ? *Sym.Size : 0;
+      DynSym.Content.add(DynStr.Content.getOffset(Sym.Name), Size, Bind,
                          convertIFSSymbolTypeToELF(Sym.Type), 0, Shndx);
     }
     DynSym.Size = DynSym.Content.getSize();
 
     // Poplulate dynamic table.
     size_t DynSymIndex = DynTab.Content.addAddr(DT_SYMTAB, 0);
     size_t DynStrIndex = DynTab.Content.addAddr(DT_STRTAB, 0);
     DynTab.Content.addValue(DT_STRSZ, DynSym.Size);
     for (const std::string &Lib : Stub.NeededLibs)

llvm/lib/InterfaceStub/IFSHandler.cpp

 };
 
 /// YAML traits for ELFSymbol.
 template <> struct MappingTraits<IFSSymbol> {
   static void mapping(IO &IO, IFSSymbol &Symbol) {
     IO.mapRequired("Name", Symbol.Name);
     IO.mapRequired("Type", Symbol.Type);
     // The need for symbol size depends on the symbol type.
     if (Symbol.Type == IFSSymbolType::NoType) {
-      IO.mapOptional("Size", Symbol.Size, (uint64_t)0);
-    } else if (Symbol.Type == IFSSymbolType::Func) {
-      Symbol.Size = 0;
-    } else {
-      IO.mapRequired("Size", Symbol.Size);
+      // Size is None, so we are reading it in, or it is non 0 so we
+      // should emit it.
+      if (!Symbol.Size || *Symbol.Size)
+        IO.mapOptional("Size", Symbol.Size);
+    } else if (Symbol.Type != IFSSymbolType::Func) {
+      IO.mapOptional("Size", Symbol.Size);
     }
     IO.mapOptional("Undefined", Symbol.Undefined, false);
     IO.mapOptional("Weak", Symbol.Weak, false);
     IO.mapOptional("Warning", Symbol.Warning);
   }
 
   // Compacts symbol information into a single line.
   static const bool flow = true; // NOLINT(readability-identifier-naming)

llvm/test/tools/llvm-ifs/strip-size.test

+# RUN: llvm-ifs %s --output-ifs - --strip-size | FileCheck %s
+
+# CHECK:      Symbols:
+# CHECK-NEXT:   - { Name: sym, Type: Object }
+# CHECK-NEXT: ...
+
+## Check that the size when emitting to elf defaults to 1.
+# RUN: llvm-ifs %s --output-elf - --strip-size | llvm-ifs - --output-ifs - | \
+# RUN:   FileCheck %s --check-prefix=ELF
+
+# ELF:      Symbols:
+# ELF-NEXT:   - { Name: sym, Type: Object, Size: 0 }
+# ELF-NEXT: ...
+
+--- !ifs-v1
+IfsVersion: 3.0
+Target: x86_64
+Symbols:
+  - { Name: sym, Type: Object, Size: 2 }
+...

llvm/tools/llvm-ifs/llvm-ifs.cpp

cl::opt<bool> StripIFSTarget(
     cl::cat(IfsCategory));
 cl::opt<bool>
     StripUndefined("strip-undefined",
                    cl::desc("Strip undefined symbols from IFS output"),
                    cl::cat(IfsCategory));
 cl::opt<bool> StripNeededLibs("strip-needed",
                               cl::desc("Strip needed libs from output"),
                               cl::cat(IfsCategory));
+cl::opt<bool> StripSize("strip-size",
+                        cl::desc("Remove object size from the output"),
+                        cl::cat(IfsCategory));
+
 cl::list<std::string>
     ExcludeSyms("exclude",
                 cl::desc("Remove symbols which match the pattern. Can be "
                          "specified multiple times"),
                 cl::cat(IfsCategory));
 
 cl::opt<std::string>
     SoName("soname",
if (OverrideError)
     fatalError(std::move(OverrideError));
 
   if (StripNeededLibs)
     Stub.NeededLibs.clear();
 
   if (Error E = filterIFSSyms(Stub, StripUndefined, ExcludeSyms))
     fatalError(std::move(E));
 
+  if (StripSize)
+    for (IFSSymbol &Sym : Stub.Symbols)
+      Sym.Size.reset();
+
   if (OutputELFFilePath.getNumOccurrences() == 0 &&
       OutputIFSFilePath.getNumOccurrences() == 0 &&
       OutputTBDFilePath.getNumOccurrences() == 0) {
     if (OutputFormat.getNumOccurrences() == 0) {
       WithColor::error() << "at least one output should be specified.";
       return -1;
     }
   } else if (OutputFormat.getNumOccurrences() == 1) {

llvm/unittests/InterfaceStub/ELFYAMLTest.cpp

TEST(ElfYamlTextAPI, YAMLReadsTBESymbols) {
   EXPECT_NE(Stub.get(), nullptr);
   EXPECT_TRUE(Stub->SoName.hasValue());
   EXPECT_STREQ(Stub->SoName->c_str(), "test.so");
   EXPECT_EQ(Stub->Symbols.size(), 5u);
 
   auto Iterator = Stub->Symbols.begin();
   IFSSymbol const &SymBar = *Iterator++;
   EXPECT_STREQ(SymBar.Name.c_str(), "bar");
-  EXPECT_EQ(SymBar.Size, 42u);
+  EXPECT_EQ(*SymBar.Size, 42u);
   EXPECT_EQ(SymBar.Type, IFSSymbolType::Object);
   EXPECT_FALSE(SymBar.Undefined);
   EXPECT_FALSE(SymBar.Weak);
   EXPECT_FALSE(SymBar.Warning.hasValue());
 
   IFSSymbol const &SymBaz = *Iterator++;
   EXPECT_STREQ(SymBaz.Name.c_str(), "baz");
-  EXPECT_EQ(SymBaz.Size, 3u);
+  EXPECT_EQ(*SymBaz.Size, 3u);
   EXPECT_EQ(SymBaz.Type, IFSSymbolType::TLS);
   EXPECT_FALSE(SymBaz.Undefined);
   EXPECT_FALSE(SymBaz.Weak);
   EXPECT_FALSE(SymBaz.Warning.hasValue());
 
   IFSSymbol const &SymFoo = *Iterator++;
   EXPECT_STREQ(SymFoo.Name.c_str(), "foo");
-  EXPECT_EQ(SymFoo.Size, 0u);
+  EXPECT_FALSE(SymFoo.Size.hasValue());
   EXPECT_EQ(SymFoo.Type, IFSSymbolType::Func);
   EXPECT_FALSE(SymFoo.Undefined);
   EXPECT_FALSE(SymFoo.Weak);
   EXPECT_TRUE(SymFoo.Warning.hasValue());
   EXPECT_STREQ(SymFoo.Warning->c_str(), "Deprecated!");
 
   IFSSymbol const &SymNor = *Iterator++;
   EXPECT_STREQ(SymNor.Name.c_str(), "nor");
-  EXPECT_EQ(SymNor.Size, 0u);
+  EXPECT_FALSE(SymNor.Size.hasValue());
   EXPECT_EQ(SymNor.Type, IFSSymbolType::NoType);
   EXPECT_TRUE(SymNor.Undefined);
   EXPECT_FALSE(SymNor.Weak);
   EXPECT_FALSE(SymNor.Warning.hasValue());
 
   IFSSymbol const &SymNot = *Iterator++;
   EXPECT_STREQ(SymNot.Name.c_str(), "not");
-  EXPECT_EQ(SymNot.Size, 111u);
+  EXPECT_EQ(*SymNot.Size, 111u);
   EXPECT_EQ(SymNot.Type, IFSSymbolType::Unknown);
   EXPECT_TRUE(SymNot.Undefined);
   EXPECT_TRUE(SymNot.Weak);
   EXPECT_TRUE(SymNot.Warning.hasValue());
   EXPECT_STREQ(SymNot.Warning->c_str(), "All fields populated!");
 }
 
 TEST(ElfYamlTextAPI, YAMLReadsNoTBESyms) {