[PATCH] D59033: [llvm-objcopy] Make .build-id linking atomic

[Jordan Rupprecht via Phabricator via llvm-commits]

rupprecht accepted this revision.
rupprecht marked an inline comment as done.
rupprecht added a comment.

Just a couple nits, overall lgtm



================
Comment at: llvm/lib/Support/Path.cpp:756
+  ResultPath = ModelStorage;
+  ResultPath.push_back(0);
+  ResultPath.pop_back();
----------------
Can you preserve the `// Null terminate.` comment here?


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:199-200
+    Path.push_back('\0');
+    return createStringError(EC, "cannot link %s to %s", ToLink.data(),
+                             Path.data());
+  }
----------------
jakehehrlich wrote:
> jhenderson wrote:
> > It may depend on the reporting site, but I believe that this will throw away the potentially useful information in `EC`. I think you need to add EC's message to the string error to avoid this.
> I looked at the implementation of createStringError. It just calls make_error which in turn calls the StringError constructor which does not include the error code unfortunately so your concern was correct. I added a function here to resolve this issue.
Wow, this is kinda a wtf. From Error.cpp:

```
StringError::StringError(std::error_code EC, const Twine &S)
    : Msg(S.str()), EC(EC) {}

StringError::StringError(const Twine &S, std::error_code EC)
    : Msg(S.str()), EC(EC), PrintMsgOnly(true) {}

void StringError::log(raw_ostream &OS) const {
  if (PrintMsgOnly) {
    OS << Msg;
  } else {
    OS << EC.message();
    if (!Msg.empty())
      OS << (" " + Msg);
  }
}
```

So, StringError(EC, "foo") will print "error code: foo", but StringError("foo", EC) will just print "foo". createStringError() uses the StringError("foo", EC) constructor.

That seems very strange. I'm totally in favor of creating a makeStringError() method here to workaround that, but we should probably fix that.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:168
+#define MODEL_16 MODEL_8 MODEL_8
+#define MODEL_32 (MODEL_16 MODEL_16)
+
----------------
Why the parens here? This expands to: "%%%%%%%%" "%%%%%%%%" "%%%%%%%%" "%%%%%%%%" which is implicitly joined together. Is it just for clarity, or is it actually required?

Either way -- either MODEL_16 should also be parens'd, or neither should be parens'd.


================
Comment at: llvm/tools/llvm-objcopy/ELF/ELFObjcopy.cpp:214
+    TmpPath.push_back('\0');
+    return makeStringError(EC, "could not remove %s", TmpPath.data());
   }
----------------
grimar wrote:
> Seems here and above you could use `SmallString::c_str()`?
> 
> It's just implemented very close to this I think:
> 
> ```
>   const char* c_str() {
>     this->push_back(0);
>     this->pop_back();
>     return this->data();
>   }
> ```
Why is it even necessary to do this here? Doesn't `createUniquePath` null terminate it?
Using c_str() seems like a good idea anyway.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59033