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

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:123
+  if (Err)
+    assert(false);
+  constexpr const char *ELF_NOTE_GNU = "GNU";
----------------
jakehehrlich wrote:
> 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.
This also probably isn't ideal - ah, here's the tool for this:

http://llvm.org/doxygen/classllvm_1_1ErrorAsOutParameter.html - hopefully that addresses your issues here, specifically "notes" should use an ErrorAsOutParameter to safely clear the error state of the callers new Error object.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:137-138
+  }
+  if (Err)
+    return std::move(Err);
+  return make_error<StringError>(
----------------
Maybe sink this into the "unwrapOrError" loop (& sink the Error itself into just before the "notes" loop) - shortest scope as needed helps make the code a bit easier to understand/reason about.


Repository:
  rL LLVM

https://reviews.llvm.org/D54384