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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/Support/FileSystem.h:770
+/// Generates a unique path suitable for a temporary file but does not
+/// open or create the file. The name is based on \a model with '%'
+/// replaced by a random char in [0-9a-f]. If \a MakeAbsolute is true
----------------
jhenderson wrote:
> model -> Model, I assume to match the variable name?
This still doesn't match the variable name?


================
Comment at: llvm/include/llvm/Support/FileSystem.h:772
+/// replaced by a random char in [0-9a-f]. If \a MakeAbsolute is true
+/// then the system's temp directory is prepended first. If \a is false
+/// the current directory will be used instead.
----------------
Rogue \a?


================
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());
+  }
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D59033