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

[Jake Ehrlich via Phabricator via llvm-commits]

jakehehrlich added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:123
+  if (Err)
+    assert(false);
+  constexpr const char *ELF_NOTE_GNU = "GNU";
----------------
rupprecht wrote:
> This should probably error instead of assert, otherwise this will cause weird failures depending on debug vs release builds
> 
> I don't really understand why it's needed from this comment either -- can you elaborate the comment?
> 
> (Probably the libobject code should just be fixed, but I don't see that as a blocker)
This is very subtle and this assert should never trigger, hence the assert. I changed it to llvm_unreachable instead. Error has state, checked vs unchecked. libObject assigns to the error rather than returning an error, but this is faulty for this reason. You can only construct an error using Error::success() but by default that is unchecked. If you assign to an unchecked Error, your program will assert fail. To avoid this, I just manually check the successful error. This is just an indication that libObject chose a bad way to propagate errors in this case. I can't really avoid it unfortunately.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:125
+  constexpr const char *ELF_NOTE_GNU = "GNU";
+  for (const auto &Phdr : unwrapOrError(In.program_headers())) {
+    if (Phdr.p_type != PT_NOTE)
----------------
rupprecht wrote:
> The ELF dumper in llvm-readobj only looks for notes in the program headers if it's a core file:
> https://github.com/llvm-mirror/llvm/blob/master/tools/llvm-readobj/ELFDumper.cpp#L3883
> Honestly I'm not sure why it does it though. Do you need to do similar logic here?
That's not really ideal behavior; ELFDumper should check both. For build ids the correct place to look is always via the program headers because only non-ET_REL binaries have build ids and such binaries will always have a PT_NOTE if they have a build id. This means stripped binaries can (even those stripped with --strip-sections) can be matched up with their build id.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:155-156
+                             const object::ELFObjectFileBase &In) {
+  Error Err = Error::success();
+  Optional<ArrayRef<uint8_t>> BuildIdBytes = findBuildID(In, Err);
+  if (Err)
----------------
dblaikie wrote:
> Curious API design - perhaps you wouldn't trip over the Error clearing issues if you used 
> 
>   Expected<ArrayRef<uint8_t>> findBuildID(In);
> 
> Errors as out parameters are, tricky.
As I have discovered by trying to deal with the one in libObject you'll see above. I've switched the code to use Expected.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:578
+    linkToBuildIdDir(Config.BuildIdLinkDir, Config.OutputFilename,
+                     Config.BuildIdLinkOutput.getValue(), In);
+  }
----------------
rupprecht wrote:
> Is the `In` here supposed to be `Out`?
No, the build id is unchanged. Thinking about it, we should probably extract the build id once and then pass the build id...not recompute it twice.


================
Comment at: llvm/tools/llvm-objcopy/ObjcopyOpts.td:169
+                         MetaVarName<"dir">;
+defm build_id_link_input : Eq<"build-id-link-input", "Hard-link the input to <dir>/xx/xxx<suffix> name derived from hex build ID">,
+                           MetaVarName<"suffix">;
----------------
rupprecht wrote:
> This looks long -- you should be able to format this automatically with `git-clang-format --extensions td master`
woah...I didn't know that was a thing!


Repository:
  rL LLVM

https://reviews.llvm.org/D54384