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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
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(
----------------
ruiu wrote:
> 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.
Can you address this comment?


================
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, \
----------------
martell wrote:
> ruiu wrote:
> > There's a off-by-one error. X6 should be X7 and so on.
> Look at `lib/ToolDrivers/llvm-lib/LibDriver.cpp#L44`
> I can change it but every infoTable in the llvm source tree has this off by 1 for the #define
> 
This is an obvious mistake, and it is not a good idea to blindly copy mistakes.


Repository:
  rL LLVM

https://reviews.llvm.org/D29892