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

[Rui Ueyama via Phabricator via llvm-commits]

ruiu added inline comments.


================
Comment at: include/llvm/Object/COFFModuleDefinition.h:45
+parseCOFFModuleDefinition(MemoryBufferRef MB, COFF::MachineTypes Machine,
+                          bool MingwDef = false);
 
----------------
martell wrote:
> ruiu wrote:
> > I don't like to add a magical boolean flag to slightly change the existing behavior. Why do you need this? This should be removed unless absolutely necessary.
> dlltool's defs are a little different then lib.exe's
> specifically mangling, see isDecorated.
> and the support for '==' for weak aliasing,
> although support for the latter won't hurt lib.exe
Why are they different?


================
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(
----------------
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.


Repository:
  rL LLVM

https://reviews.llvm.org/D29892