[PATCH] D109972: Regenerate LC_CODE_SIGNATURE during llvm-objcopy operations

Fri Sep 17 17:03:26 PDT 2021

alexander-shaposhnikov added a comment.

Hi

================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:441
+    }
+    object::CodeSignatureSection SignatureBuilder =
+        object::CodeSignatureSection(
----------------
It looks like I've missed the previous diff https://reviews.llvm.org/D109803, sorry about being late.

1/ MachO.h is probably not the ideal place for this functionality, according to the top file comment "MachO.h declares the MachOObjectFile class". My first impression is that the public declaration probably should be placed into a separate file, e.g. "MachOCodeSignature.h" (and we need a detailed comment explaining what this class or function does)

2/ The responsibility of the class CodeSignatureSection seems to be somewhat unclear. Perhaps, we don't really need a class here at all. What would you say to just creating a function, e.g. std::string buildCodeSignatureStab(...) or void writeCodeSignatureStab(...) with some clear separation of the input / output and removing the unrelated pieces of functionality (e.g. msync on the buffer, this is probably not suitable for a library function) ? (and not expose the implementation details in the header file).

3/ Some minor comments, e.g. stripOutputFilePath - I suspect Suport/Path.h might already have some helper utilities

What I would recommend here - can we (temporarily) take a step back and  reopen & revert D109803, if it were not a library interface I would not be so worried, but for some common and frequently used libraries like libObject I would highly suggest we should clean up the interface first and I'm more than happy to help.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109972