[PATCH] D97600: [lld-macho] Implement options -rename_section -rename_segment

[Jez Ng via Phabricator via llvm-commits]

int3 added inline comments.


================
Comment at: lld/MachO/Config.h:29-30
+using NamePair = std::pair<llvm::StringRef, llvm::StringRef>;
+using SectionRenameMap = std::map<NamePair, NamePair>;
+using SegmentRenameMap = std::map<llvm::StringRef, llvm::StringRef>;
+
----------------
try to avoid `std::map` in general (as per the [[ https://llvm.org/docs/ProgrammersManual.html#map-like-containers-std-map-densemap-etc | LLVM programmer's manual ]]). In this case we could use a DenseMap of NamePair and a DenseMap of CachedHashStringRef respectively


================
Comment at: lld/MachO/Config.h:87-94
+  NamePair maybeRenameSection(NamePair key) const {
+    auto newNames = sectionRenameMap.find(key);
+    if (newNames != sectionRenameMap.end())
+      return newNames->second;
+    auto newName = segmentRenameMap.find(key.first);
+    if (newName != segmentRenameMap.end())
+      return std::make_pair(newName->second, key.second);
----------------
`Config` should be just a plain bag of values. I think this can be a standalone function in Writer.cpp


================
Comment at: lld/MachO/Driver.cpp:822
+  auto validName = [invalidNameChars](StringRef s) {
+    if (s.find_first_of(invalidNameChars) != std::string::npos)
+      error("invalid name for segment or section: " + s);
----------------



================
Comment at: lld/MachO/InputFiles.cpp:109
   auto *hdr = reinterpret_cast<const MachO::fat_header *>(buf);
-  if (read32be(&hdr->magic) != MachO::FAT_MAGIC) {
+  if (mbref.getBufferSize() < sizeof(hdr->magic) ||
+      read32be(&hdr->magic) != MachO::FAT_MAGIC) {
----------------
gkm wrote:
> FYI: This turned-up in my option-parsing test: when I omitted a sub-arg, then it swallowed `-o`, leaving `/dev/null` without a prefix, so it was interpreted as an input filename. Since its size was zero, ASAN caught the attempted read of the non-existent header.
Ah, nice catch. This should probably be in a separate diff with its own test. Also, I think we should make it an error if the file is shorter than `sizeof(hdr->magic)`... one issue though is that we currently use `readFile` to read non-binary files like the symbol order file, but I think we can just create a separate function for that


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D97600