[PATCH] D33880: COFF: Introduce LD shim around LINK
Rui Ueyama via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Jun 5 06:34:17 PDT 2017
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
More information about the llvm-commits
mailing list