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

[Daniel Rodríguez Troitiño via Phabricator via llvm-commits]

drodriguez added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/MachO/MachOWriter.cpp:441
+    }
+    object::CodeSignatureSection SignatureBuilder =
+        object::CodeSignatureSection(
----------------
alexander-shaposhnikov wrote:
> 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.
> 
> 
> 
Hi Alex,

We would prefer to fix forward instead of reverting, if possible. As it sits, D109803 haven't changed anything and it is just "dormant" code as far as LLVM respects. Only this diff uses that functionality.

I will review your comment next week more carefully and provide better answers.

1/ I think we can do this. It should not be a problem.
2/ The class tries to imitate the existing code in LLD, which was a class. There's two related functionalities to the class: measuring the size, and serializing. For both one needs the same inputs, so it was interesting to tie data and behaviour together.
2b/ `msync` on the buffer is actually important, and `llvm-objcopy` might not work unless we make that call. IIRC without that, if one was writing the same file, and that file was mmap'ed into memory, the kernel will not realize that the signature had changed, and will not evaluate again. I can look for a reference that explained the problem in a little bit more detail.
3/ We realize that it was not portable for Windows, and Nuri changed that this afternoon to use `llvm::path` instead. This diff has the fix, since the previous one was already landed.

Thanks for the help offering. Any feedback is welcome. We will try to get to the best version of this that satisfies everyone's requirements.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D109972