[objcopy][NFC] Move NameOrPattern::create() into CommonConfig.h

[avl]

Summary

While moving objcopy into separate library(D88827), NameOrPattern::create()
was mistakenly placed into ObjcopyOptions.cpp. This patch moves
the NameOrPattern::create() into CommonConfig.h. Additionally it adds
test for using NameOrPattern.

[jhenderson]

llvm/include/llvm/ObjCopy/CommonConfig.h

113–115

Given the complexity of this function, I think it should be in a .cpp file.

llvm/unittests/ObjCopy/ObjCopyTest.cpp

369–372

It seems to me like this function has a lot of copied code in it. I think you should be able to share most of it rather than just copying.

382

439

If this YAML is identical to other cases, pull it into a shared constant somewhere.

[avl]

addressed comments(moved NameOrPattern::create() into .cpp, splitted functions into several smaller in ObjCopyTest.cpp)

[avl]

llvm/unittests/ObjCopy/ObjCopyTest.cpp

439

I suggest to not use shared constants here. These are different places(even if yaml descriptions look similar). Changing in one place should not necessarily be reflected in others. Also it is more convenient to see them inplace rather then moved to constant definition.

Though if you still think we need shared constants here - will update the patch accordingly.

[jhenderson]

llvm/lib/ObjCopy/CommonConfig.cpp

16 ↗

(On Diff #413431)

I'm not sure you need the llvm:: prefix.

llvm/unittests/ObjCopy/ObjCopyTest.cpp

56

Nit here and below: LLVM style says use loweer case for start of error messages.

66

Sec is a more common abbreviation for "section". Applies elsewhere too.

67

79

102

Maybe?

387

Do you need the trailing return type?

439

I think it should be shared: if at some point it needs to be unshared, people can make the required change then. Similarly, future developers should be careful not to make changes to shared code without understanding the implications.

As a direct counterpoint to your comment: what if e.g. the YAML format changed slightly, necessitating a change to the input strings: you wouldn't want to modify the same s tring multiple timesm would you?

[avl]

llvm/unittests/ObjCopy/ObjCopyTest.cpp

439

I think it should be shared: if at some point it needs to be unshared, people can make the required change then. Similarly, future developers should be careful not to make changes to shared code without understanding the implications.

Ok, would change to shared constants.

As a direct counterpoint to your comment: what if e.g. the YAML format changed slightly, necessitating a change to the input strings: you wouldn't want to modify the same s tring multiple timesm would you?

That would be mechanical search&replace. It looks not a problem from my point of view.

[jhenderson]

llvm/unittests/ObjCopy/ObjCopyTest.cpp

439

That would be mechanical search&replace. It looks not a problem from my point of view.

Assuming the developer knew there were multiple places to change and the nature of the change is straightforward enough to be amenable to that.

[avl]

addressed comments.

[jhenderson]

LGTM, with one suggestion.

llvm/unittests/ObjCopy/ObjCopyTest.cpp

26

Simpler name: SimpleFileCOFFYAML. Similar for the others.

[avl]

Ok. Thanks!

llvm/include/llvm/ObjCopy/CommonConfig.h

class NameOrPattern {
   NameOrPattern(StringRef N) : Name(N) {}
   NameOrPattern(std::shared_ptr<Regex> R) : R(R) {}
   NameOrPattern(std::shared_ptr<GlobPattern> G, bool IsPositiveMatch)
       : G(G), IsPositiveMatch(IsPositiveMatch) {}
 
 public:
   // ErrorCallback is used to handle recoverable errors. An Error returned
   // by the callback aborts the parsing and is then returned by this function.
-  static Expected<NameOrPattern>
+  static inline Expected<NameOrPattern>
   create(StringRef Pattern, MatchStyle MS,
-         llvm::function_ref<Error(Error)> ErrorCallback);
+         llvm::function_ref<Error(Error)> ErrorCallback) {
+    switch (MS) {
+    case MatchStyle::Literal:
+      return NameOrPattern(Pattern);
+    case MatchStyle::Wildcard: {
+      SmallVector<char, 32> Data;
+      bool IsPositiveMatch = true;
+      if (Pattern[0] == '!') {
+        IsPositiveMatch = false;
+        Pattern = Pattern.drop_front();
+      }
+      Expected<GlobPattern> GlobOrErr = GlobPattern::create(Pattern);
+
+      // If we couldn't create it as a glob, report the error, but try again
+      // with a literal if the error reporting is non-fatal.
+      if (!GlobOrErr) {
+        if (Error E = ErrorCallback(GlobOrErr.takeError()))
+          return std::move(E);
+        return create(Pattern, MatchStyle::Literal, ErrorCallback);
+      }
+
+      return NameOrPattern(std::make_shared<GlobPattern>(*GlobOrErr),
+                           IsPositiveMatch);
+    }
+    case MatchStyle::Regex: {
+      SmallVector<char, 32> Data;
+      return NameOrPattern(std::make_shared<Regex>(
+          ("^" + Pattern.ltrim('^').rtrim('$') + "$").toStringRef(Data)));
+    }
+    }
+    llvm_unreachable("Unhandled llvm.objcopy.MatchStyle enum");
+  }
 
   bool isPositiveMatch() const { return IsPositiveMatch; }
   Optional<StringRef> getName() const {
     if (!R && !G)
       return Name;
     return None;
   }
   bool operator==(StringRef S) const {

llvm/tools/llvm-objcopy/ObjcopyOptions.cpp

if (!TrimmedLine.empty())
       if (Error E = Symbols.addMatcher(NameOrPattern::create(
               Saver.save(TrimmedLine), MS, ErrorCallback)))
         return E;
   }
 
   return Error::success();
 }
 
-Expected<NameOrPattern>
-NameOrPattern::create(StringRef Pattern, MatchStyle MS,
-                      function_ref<Error(Error)> ErrorCallback) {
-  switch (MS) {
-  case MatchStyle::Literal:
-    return NameOrPattern(Pattern);
-  case MatchStyle::Wildcard: {
-    SmallVector<char, 32> Data;
-    bool IsPositiveMatch = true;
-    if (Pattern[0] == '!') {
-      IsPositiveMatch = false;
-      Pattern = Pattern.drop_front();
-    }
-    Expected<GlobPattern> GlobOrErr = GlobPattern::create(Pattern);
-
-    // If we couldn't create it as a glob, report the error, but try again with
-    // a literal if the error reporting is non-fatal.
-    if (!GlobOrErr) {
-      if (Error E = ErrorCallback(GlobOrErr.takeError()))
-        return std::move(E);
-      return create(Pattern, MatchStyle::Literal, ErrorCallback);
-    }
-
-    return NameOrPattern(std::make_shared<GlobPattern>(*GlobOrErr),
-                         IsPositiveMatch);
-  }
-  case MatchStyle::Regex: {
-    SmallVector<char, 32> Data;
-    return NameOrPattern(std::make_shared<Regex>(
-        ("^" + Pattern.ltrim('^').rtrim('$') + "$").toStringRef(Data)));
-  }
-  }
-  llvm_unreachable("Unhandled llvm.objcopy.MatchStyle enum");
-}
-
 static Error addSymbolsToRenameFromFile(StringMap<StringRef> &SymbolsToRename,
                                         BumpPtrAllocator &Alloc,
                                         StringRef Filename) {
   StringSaver Saver(Alloc);
   SmallVector<StringRef, 16> Lines;
   auto BufOrErr = MemoryBuffer::getFile(Filename);
   if (!BufOrErr)
     return createFileError(Filename, BufOrErr.getError());

llvm/unittests/ObjCopy/ObjCopyTest.cpp

 
 using namespace llvm;
 using namespace object;
 using namespace objcopy;
 using namespace yaml;
 
 void copySimpleInMemoryFileImpl(
     const char *YamlCreationString,
     std::function<bool(const Binary &File)> IsValidFormat) {
   auto ErrHandler = [&](const Twine &Msg) { FAIL() << "Error: " << Msg; };
 
   // Create Object file from YAML description.
   SmallVector<char> Storage;
   std::unique_ptr<ObjectFile> Obj =
       yaml2ObjectFile(Storage, YamlCreationString, ErrHandler);
   ASSERT_TRUE(Obj);
   ASSERT_TRUE(IsValidFormat(*Obj));
void copySimpleInMemoryFileImpl(
   // Check copied file.
   Expected<std::unique_ptr<Binary>> Result = createBinary(Buffer);
   ASSERT_THAT_EXPECTED(Result, Succeeded());
   ASSERT_TRUE(IsValidFormat(**Result));
 }
 
 TEST(CopySimpleInMemoryFile, COFF) {
   SCOPED_TRACE("CopySimpleInMemoryFileCOFF");
 
   copySimpleInMemoryFileImpl(
       R"(
 --- !COFF
 header:
   Machine:         IMAGE_FILE_MACHINE_AMD64
   Characteristics: [  ]
 sections:
   - Name:            .text
     Characteristics: [  ]
     Alignment:       4
     SectionData:     E800000000C3C3C3
 symbols:
 ...
 )",
       [](const Binary &File) { return File.isCOFF(); });
 }
 
 TEST(CopySimpleInMemoryFile, ELF) {
   SCOPED_TRACE("CopySimpleInMemoryFileELF");
 
   copySimpleInMemoryFileImpl(
       R"(
 --- !ELF
 FileHeader:
    Class:    ELFCLASS64
    Data:     ELFDATA2LSB
    Type:     ET_REL)",
       [](const Binary &File) { return File.isELF(); });
 }
 
 TEST(CopySimpleInMemoryFile, MachO) {
   SCOPED_TRACE("CopySimpleInMemoryFileMachO");
 
   copySimpleInMemoryFileImpl(
       R"(
 --- !mach-o
 FileHeader:
   magic:           0xFEEDFACF
   cputype:         0x01000007
   cpusubtype:      0x80000003
   filetype:        0x00000002
   ncmds:           0
   sizeofcmds:      0
   flags:           0x00218085
   reserved:        0x00000000
 ...
 )",
       [](const Binary &File) { return File.isMachO(); });
 }
 
 TEST(CopySimpleInMemoryFile, Wasm) {
   SCOPED_TRACE("CopySimpleInMemoryFileWasm");
 
   copySimpleInMemoryFileImpl(
Sections:
         reserved1:       0x00000000
         reserved2:       0x00000000
         reserved3:       0x00000000
 ...
 )",
       [](const Binary &File) { return File.isMachO(); }, "__foo", "1234",
       UpdateSection);
 }
+
+void removeSectionByPatternImpl(
+    const char *YamlCreationString,
+    std::function<bool(const Binary &File)> IsValidFormat,
+    StringRef SectionWildcard, StringRef SectionName) {
+  auto ErrHandler = [&](const Twine &Msg) { FAIL() << "Error: " << Msg; };
+
+  // Create Object file from YAML description.
+  SmallVector<char> Storage;
+  std::unique_ptr<ObjectFile> Obj =
+      yaml2ObjectFile(Storage, YamlCreationString, ErrHandler);
+  ASSERT_TRUE(Obj);
+  ASSERT_TRUE(IsValidFormat(*Obj));
+
+  // Check that section is presented.
+  bool HasSection = false;
+  for (const object::SectionRef &Sect : (*Obj).sections()) {
+    Expected<StringRef> SectNameOrErr = Sect.getName();
+    ASSERT_THAT_EXPECTED(SectNameOrErr, Succeeded());
+
+    if ((*SectNameOrErr) == SectionName) {
+      HasSection = true;
+      break;
+    }
+  }
+  EXPECT_TRUE(HasSection);
+
+  Expected<NameOrPattern> Pattern = objcopy::NameOrPattern::create(
+      SectionWildcard, objcopy::MatchStyle::Wildcard,
+      [](Error Err) -> Error { return Err; });
+
+  ConfigManager Config;
+  Config.Common.OutputFilename = "a.out";
+  EXPECT_THAT_ERROR(Config.Common.ToRemove.addMatcher(std::move(Pattern)),
+                    Succeeded());
+
+  // Call executeObjcopyOnBinary()
+  SmallVector<char> DataVector;
+  raw_svector_ostream OutStream(DataVector);
+  Error Err = objcopy::executeObjcopyOnBinary(Config, *Obj.get(), OutStream);
+  ASSERT_FALSE(std::move(Err));
+
+  MemoryBufferRef Buffer(StringRef(DataVector.data(), DataVector.size()),
+                         Config.Common.OutputFilename);
+
+  // Check copied file.
+  Expected<std::unique_ptr<Binary>> Result = createBinary(Buffer);
+  ASSERT_THAT_EXPECTED(Result, Succeeded());
+  ASSERT_TRUE(IsValidFormat(**Result));
+  ASSERT_TRUE((*Result)->isObject());
+
+  // Check that section was removed.
+  HasSection = false;
+  for (const object::SectionRef &Sect :
+       static_cast<ObjectFile *>((*Result).get())->sections()) {
+    Expected<StringRef> SectNameOrErr = Sect.getName();
+    ASSERT_THAT_EXPECTED(SectNameOrErr, Succeeded());
+
+    if ((*SectNameOrErr) == SectionName) {
+      HasSection = true;
+      break;
+    }
+  }
+  EXPECT_TRUE(!HasSection);
+}
+
+TEST(RemoveSectionByPattern, COFF) {
+  SCOPED_TRACE("removeSectionByPatternCOFF");
+
+  removeSectionByPatternImpl(
+      R"(
+--- !COFF
+header:
+  Machine:         IMAGE_FILE_MACHINE_AMD64
+  Characteristics: [  ]
+sections:
+  - Name:            .foo
+    Characteristics: [  ]
+    Alignment:       4
+    SectionData:     E800000000C3C3C3
+symbols:
+...
+)",
+      [](const Binary &File) { return File.isCOFF(); }, "\\.foo*", ".foo");
+}
+
+TEST(RemoveSectionByPattern, ELF) {
+  SCOPED_TRACE("removeSectionByPatternELF");
+
+  removeSectionByPatternImpl(
+      R"(
+--- !ELF
+FileHeader:
+  Class:    ELFCLASS64
+  Data:     ELFDATA2LSB
+  Type:     ET_REL
+Sections:
+  - Name:            .foo
+    Type:            SHT_PROGBITS
+    Flags:           [ SHF_ALLOC ]
+    Content:        "12345678"
+)",
+      [](const Binary &File) { return File.isELF(); }, "\\.foo*", ".foo");
+}
+
+TEST(RemoveSectionByPattern, MachO) {
+  SCOPED_TRACE("removeSectionByPatternMachO");
+
+  removeSectionByPatternImpl(
+      R"(
+--- !mach-o
+FileHeader:
+  magic:           0xFEEDFACF
+  cputype:         0x01000007
+  cpusubtype:      0x80000003
+  filetype:        0x00000001
+  ncmds:           1
+  sizeofcmds:      152
+  flags:           0x00002000
+  reserved:        0x00000000
+LoadCommands:
+  - cmd:             LC_SEGMENT_64
+    cmdsize:         152
+    segname:         __TEXT
+    vmaddr:          0
+    vmsize:          4
+    fileoff:         184
+    filesize:        4
+    maxprot:         7
+    initprot:        7
+    nsects:          1
+    flags:           0
+    Sections:
+      - sectname:        __foo
+        segname:         __TEXT
+        addr:            0x0000000000000000
+        content:         'AABBCCDD'
+        size:            4
+        offset:          184
+        align:           0
+        reloff:          0x00000000
+        nreloc:          0
+        flags:           0x80000400
+        reserved1:       0x00000000
+        reserved2:       0x00000000
+        reserved3:       0x00000000
+...
+)",
+      [](const Binary &File) { return File.isMachO(); }, "__TEXT,__foo*",
+      "__foo");
+}
+
+TEST(RemoveSectionByPattern, Wasm) {
+  SCOPED_TRACE("removeSectionByPatternWasm");
+
+  removeSectionByPatternImpl(
+      R"(
+--- !WASM
+FileHeader:
+  Version:         0x00000001
+Sections:
+  - Type: CUSTOM
+    Name: foo
+    Payload: ABC123
+...
+)",
+      [](const Binary &File) { return File.isWasm(); }, "foo*", "foo");
+}