[PATCH] D33880: COFF: Introduce LD shim around LINK

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added a comment.

- `lib` is not the right place to put these files. Create a directory `mingw` as `lld/mingw` and move new files to that directory.



================
Comment at: lld/lib/Shim/COFFLdOptions.td:1
+include "llvm/Option/OptParser.td"
+
----------------
Rename this `Options.td`.


================
Comment at: lld/lib/Shim/COFFLdShim.cpp:1
+//===- lib/Driver/COFFLdShim.cpp ------------------------------------------===//
+//
----------------
martell wrote:
> Location Okay?
See the comment above.

Rename this file `MingwDriver.cpp`.


================
Comment at: lld/lib/Shim/COFFLdShim.cpp:12
+///
+/// ld shim around the COFF link frontend.
+///
----------------
`This is a linker driver for MinGW.`


================
Comment at: lld/lib/Shim/COFFLdShim.cpp:16-17
+
+#include "lld/Driver/Driver.h"
+#include "lld/Driver/Driver.h"
+#include "llvm/ADT/ArrayRef.h"
----------------
Remove duplicate.


================
Comment at: lld/lib/Shim/COFFLdShim.cpp:65
+// Create OptTable class for parsing actual command line arguments
+class COFFLdOptTable : public llvm::opt::OptTable {
+public:
----------------
`COFFLdOptTable` -> `MingwOptTable`.


================
Comment at: lld/lib/Shim/COFFLdShim.cpp:73
+namespace lld {
+namespace coff {
+
----------------
You should create a new namespace `lld::mingw`.


================
Comment at: lld/lib/Shim/COFFLdShim.cpp:77
+static std::vector<StringRef> SearchPaths;
+static llvm::StringRef Sysroot;
+
----------------
Remove all occurences of `llvm::`.


================
Comment at: lld/lib/Shim/COFFLdShim.cpp:80
+static void error(const Twine &Msg) {
+  llvm::errs() << Msg << "\n";
+  exit(1);
----------------
Remove `llvm::`.


================
Comment at: lld/lib/Shim/COFFLdShim.cpp:92
+    path::append(S, Path1, Path2);
+  if (fs::exists(S)) {
+    return S.str().str();
----------------
Remove `{}`.


================
Comment at: lld/lib/Shim/COFFLdShim.cpp:107
+// search paths.
+Optional<std::string> searchLibrary(StringRef Name, bool IgnoreShared = false) {
+  if (Name.startswith(":"))
----------------
Remove `IgnoreShared` as you are not using it.



================
Comment at: lld/lib/Shim/COFFLdShim.cpp:121
+void addFile(StringRef Path) {
+  std::string *P = new std::string(Path);
+  LinkArgs.push_back(P->c_str());
----------------
Don't new `std::string` -- you are leaking memory.



================
Comment at: lld/lib/Shim/COFFLdShim.cpp:144
+  }
+  // Let the Link Driver handle this error?
+  //if (Files.empty() && ErrorCount == 0)
----------------
martell wrote:
> Do I let LINK.exe handle such errors?
Either is probably fine, but don't leave this kind of comment.


================
Comment at: lld/lib/Shim/COFFLdShim.cpp:163
+
+static bool directMap(opt::InputArgList &Args, unsigned Key, StringRef outArg) {
+  StringRef S = getString(Args, Key);
----------------
Maybe `forward` is a better name.

Don't return true/false for no reason. You are not using the return value, so return `void`.


================
Comment at: lld/lib/Shim/COFFLdShim.cpp:173
+
+static bool directMapValue(opt::InputArgList &Args, unsigned Key, StringRef cmpArg,
+                           StringRef outArg, StringRef outValueArg) {
----------------
Don't return a value unless you are actually using it.


================
Comment at: lld/lib/Shim/COFFLdShim.cpp:176
+  StringRef S = getString(Args, Key);
+  if(S == cmpArg) {
+    std::string *a = new std::string(Twine("-" + outArg+ ":" + outValueArg).str());
----------------
Can you run clang-format before sending a new patch to review, please?


================
Comment at: lld/lib/Shim/COFFLdShim.cpp:177
+  if(S == cmpArg) {
+    std::string *a = new std::string(Twine("-" + outArg+ ":" + outValueArg).str());
+    LinkArgs.push_back(a->c_str());
----------------
Memory leak.


================
Comment at: lld/tools/lld/lld.cpp:52
 
+static bool isPETarget(const std::vector<const char *> &V) {
+  for (auto It = V.begin(); It != V.end(); It++) {
----------------
`isMingw`


================
Comment at: lld/tools/lld/lld.cpp:53
+static bool isPETarget(const std::vector<const char *> &V) {
+  for (auto It = V.begin(); It != V.end(); It++) {
+    if (*It == StringRef("-m")) {
----------------
  for (auto It = V.begin(); It + 1 != V.end(); ++It) {
    if (StringRef(*It) != "-m")
      continue;
    StringRef S = *(It + 1);
    return S == "i386pe" || S == "i386pep" || S == "thumb2pe";
  }
  return false;



================
Comment at: lld/tools/lld/lld.cpp:123
   case Gnu:
-    return !elf::link(Args, true);
+    if (isPETarget(Args))
+      return !coff::ldshim(Args);
----------------
martell wrote:
> I'm not quite sure if I should do this here or in the ELF's `Driver.cpp` where it checks the emulation.
> Suggestions welcome
Doing this right here is the correct approach


================
Comment at: lld/tools/lld/lld.cpp:125
+      return !coff::ldshim(Args);
+    else
+      return !elf::link(Args, true);
----------------
Remove `else` because the last `if` ends with `return`.


Repository:
  rL LLVM

https://reviews.llvm.org/D33880