[PATCH] [PECOFF] Handle .drectve section.

[Rui Ueyama]

  Forgot to publish these comments.


================
Comment at: lib/ReaderWriter/PECOFF/ReaderCOFF.cpp:415
@@ +414,3 @@
+        return ec;
+      _directives = std::move(std::string(
+          reinterpret_cast<const char *>(&contents[0])));
----------------
Reid Kleckner wrote:
> Is it worth doing any kind of check that the contents are string-like and don't contain null bytes?
Good point. There seems to be actually no guarantee that the string in the section is terminated by NUL.

================
Comment at: lib/ReaderWriter/PECOFF/ReaderCOFF.cpp:444
@@ -398,1 +443,3 @@
 
+class StringSetSaver : public llvm::cl::StringSaver {
+public:
----------------
Reid Kleckner wrote:
> This isn't saving strings in a set, it's saving them in a BumpPtrAllocator.
Renamed.

================
Comment at: lib/ReaderWriter/PECOFF/ReaderCOFF.cpp:447
@@ +446,3 @@
+  virtual const char *SaveString(const char *str) {
+    auto *res = new (_alloc) std::string(str);
+    return res->c_str();
----------------
Reid Kleckner wrote:
> Will BumpPtrAllocator call dtors of objects created like this?  What I had in mind as a possible implementation was _alloc.Allocate(strlen(str)+1) plus a memcpy.
Done.

================
Comment at: lib/ReaderWriter/PECOFF/ReaderCOFF.cpp:500
@@ +499,3 @@
+    llvm::raw_string_ostream stream(errorMessage);
+    bool parseFailed = WinLinkDriver::parse(argc, argv, *targetInfo, stream);
+    stream.flush();
----------------
Reid Kleckner wrote:
> Does this save references to any Arg or ArgList objects?  I'm assuming it doesn't.  An Arg can hold a pointer to an argv string, which would be bad here.
By Arg which variable are you pointing to? Not sure if this can be an answer, but the TargetInfo will keep references to arguments if arguments are filename.

================
Comment at: lib/ReaderWriter/PECOFF/ReaderCOFF.cpp:543
@@ -437,2 +542,3 @@
   ReaderArchive _readerArchive;
+  mutable StringSetSaver _stringSaver;
 };
----------------
Reid Kleckner wrote:
> This should probably have the same lifetime as argv and thus be a local in handleDirectiveSection.
Because of the above reason it should have the same lifetime as the targetinfo.


http://llvm-reviews.chandlerc.com/D1246