[PATCH] D54384: [llvm-objcopy] Add --build-id-link-dir flag

[James Henderson via Phabricator via llvm-commits]

jhenderson added a comment.

LGTM, with a couple of minor comments.



================
Comment at: llvm/test/tools/llvm-objcopy/build-id-link-dir.test:11
+
+# RUN: llvm-objcopy --build-id-link-dir=%t-dir --build-id-link-output= --strip-sections %t %t4
+# RUN: cmp %t-dir/4f/cb712aa6387724a9f465a32cd8c14b %t4
----------------
jhenderson wrote:
> jakehehrlich wrote:
> > jhenderson wrote:
> > > `--build-id-link-output= ...` looks weird and I don't think should be valid syntax. Could it not just be `--build-id-link-output`?
> > > 
> > > If specifying an output is valid, there should be a test for that...
> > > 
> > > Basically, I'm a little confused as to what is going on here and in the next test.
> > Setting an output is valid. It's just that stripped binaries traditionally have no extension. an empty "=" is commonly used to pass an empty string and is valid for all "=" arguments across llvm from the argument parser's perspective. This is equivalent to `--build-id-link-output ""`
> > 
> > The imagined use case for --build-idlink-output that uses a non-empty string would be if you're not stripping, you're adding or removing something and you need ".debug" afterwards. Or using "--only-keep-debug" despite that not having a proper implementation at this exact moment. The example for this would be `llvm-objcopy --build-id-link-output=.debug --only-keep-debug <input> <output>` there's also this informally proposed idea that has floated around a few people here of adding a third ".dwp" file to the build id directory but I think people wanted to put that feature directly in llvm-dwp. Also in that case you're more or less directly using llvm-objcopy just to do the copy to the build id directory and no other operation.
> The weird syntax still exists: trailing '=' looks like something is missing. I'd prefer a solution to it. Either --build-id-link-output should be a valid switch on its own with no argument, as well as --build-id-link-output=<some value> or some other solution si needed.
Ah, okay that makes more sense now. Thanks. "" is a bit clearer to me, I guess.


================
Comment at: llvm/test/tools/llvm-objcopy/no-build-id2.test:1
+# RUN: yaml2obj %s > %t
+# RUN: not llvm-objcopy --build-id-link-dir=%t-dir --build-id-link-input=.debug %t 2>&1 >/dev/null | FileCheck %s
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > Why is this test a separate test? Perhaps naming it differently would help clarify.
> The intent was for one to have notes but no build id entry and the other to just not have notes. Those would need two different yaml files. I guess I forgot to add that. They've been added now.
Okay. Maybe rename "no-build-id2.test" to "no-build-id-no-notes.test"? Not too bothered though. Alternatively, a comment in the two files explaining the difference.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:125
+    llvm_unreachable("This error should have been a success");
+  constexpr const char *ELF_NOTE_GNU = "GNU";
+  for (const auto &Phdr : unwrapOrError(In.program_headers())) {
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > Why the constexpr here? Pretty sure it's not going to achieve much. Also, I don't think this is the correct case for constants in LLVM - I think it's supposed to be the same as other local variables.
> In other code bases I've been working in constexpr has been prefered. As for the case correctness. This is a defined constant in system's <elf.h> but not in llvm's BinaryFormat header for elf. I'm adding it here as it would normally appear in the header. I'll remove the constexpr but I'd like to keep the name.
Could we not just add it to the header?


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54384