[PATCH] D29892: ar: add llvm-dlltool support

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: lib/Object/ArchiveWriter.cpp:322
+      if (Symflags & object::SymbolRef::SF_Undefined &&
+        !(Symflags & object::SymbolRef::SF_Indirect))
         continue;
----------------
Indentation. Please use clang-format-diff to format your patch before requesting code review.


================
Comment at: lib/Object/COFFImportFile.cpp:584-585
+    if (E.isWeak()) {
+      std::vector<uint8_t> *WeakBuffer1 = new std::vector<uint8_t>();
+      std::vector<uint8_t> *WeakBuffer2 = new std::vector<uint8_t>();
+      Members.push_back(
----------------
martell wrote:
> ruiu wrote:
> > Please don't use raw `new`s. You really want to avoid doing that, not only in this patch, but in other patches as well. You are leaking memory. I believe you are doing this because otherwise you would be accessing free'd memory, but leaking memory is also a bug which is not acceptable. Using `new` is not a solution for a use-after-free bug.
> Sorry about that Rui, I didn't even realise I used `new` here.
> This doesn't even need `new` because it is pushed back onto the vector.
> Updating the patch now. Also fixed and tested with mingw-w64.
Why do you have to pass buffers in the first place? Their lifetime is till the end of this block, so it seems unnecessary.


================
Comment at: lib/Object/COFFModuleDefinition.cpp:59
+static bool isDecorated(StringRef Sym, bool MingwDef) {
+  // Disable Sym.startswith("_") for mingw-w64
+  return (!MingwDef && Sym.startswith("_")) || Sym.startswith("@") ||
----------------
... because mingw doesn't prepend "_".


================
Comment at: lib/Object/COFFModuleDefinition.cpp:87
     case '=':
+      // This lets us handle  "==" which is a dlltool ism
+      if (Buf[1] == '=')
----------------
Please format comment. This comment has two space characters. Please add a full stop at end of a sentence.


================
Comment at: lib/Object/COFFModuleDefinition.cpp:222-227
     if (Machine == IMAGE_FILE_MACHINE_I386) {
-      if (!isDecorated(E.Name))
+      if (!isDecorated(E.Name, MingwDef))
         E.Name = (std::string("_").append(E.Name));
-      if (!E.ExtName.empty() && !isDecorated(E.ExtName))
+      if (!E.ExtName.empty() && !isDecorated(E.ExtName, MingwDef))
         E.ExtName = (std::string("_").append(E.ExtName));
     }
----------------
Do you think you can just do this?

    if (Machine == IMAGE_FILE_MACHINE_I386 && !MingwDef) {
      if (!isDecorated(E.Name))
        E.Name = (std::string("_").append(E.Name));
      if (!E.ExtName.empty() && !isDecorated(E.ExtName))
        E.ExtName = (std::string("_").append(E.ExtName));
    }



================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:53
+static const llvm::opt::OptTable::Info infoTable[] = {
+#define OPTION(X1, X2, ID, KIND, GROUP, ALIAS, X6, X7, X8, X9, X10, X11)       \
+  {X1, X2, X9,          X10,         OPT_##ID, llvm::opt::Option::KIND##Class, \
----------------
There's a off-by-one error. X6 should be X7 and so on.


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:112-115
+    llvm::errs() << "missing arg value for \""
+                 << Args.getArgString(MissingIndex) << "\", expected "
+                 << MissingCount
+                 << (MissingCount == 1 ? " argument.\n" : " arguments.\n");
----------------
Let's simplify this.

  llvm::errs() << Args.getArgString(MissingIndex) + ": missing argument\n";


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:119-123
+  if (Args.hasArgNoClaim(OPT_INPUT)) {
+    llvm::errs() << "Unknown usage : " << ArgsArr[0]
+                 << "currently only supports import lib creation\n";
+    return 1;
+  }
----------------
Merge this with the following `if` so that it just prints out the help message.


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:128
+    Table.PrintHelp(outs(), ArgsArr[0], "dlltool", false);
+    outs() << "\nTARGETS: ";
+    printTargets();
----------------
It is easier to just add `i386, i386:x86-64, arm` at end of this line


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:134
+  if (!Args.hasArgNoClaim(OPT_m) && Args.hasArgNoClaim(OPT_d)) {
+    llvm::errs() << "error: No target machine specified\n";
+    llvm::errs() << "supported targets: ";
----------------
No -> no


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:135
+    llvm::errs() << "error: No target machine specified\n";
+    llvm::errs() << "supported targets: ";
+    printTargets();
----------------
and this line.


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:141
+  for (auto *Arg : Args.filtered(OPT_UNKNOWN))
+    llvm::errs() << "ignoring unknown argument: " << Arg->getSpelling() << "\n";
+
----------------
Remove `llvm::` as you did `using namespace llvm`.


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:166
+
+  // has to happen after the parser
+  if (auto *Arg = Args.getLastArg(OPT_D))
----------------
Why?


================
Comment at: lib/ToolDrivers/llvm-dlltool/DlltoolDriver.cpp:173-175
+  if (!writeImportLibrary(Def->OutputFile, Path, Def->Exports, Machine))
+    return 0;
+  return 1;
----------------
Reverse the condition and return 0 at end of main.


Repository:
  rL LLVM

https://reviews.llvm.org/D29892