[PATCH] D37240: Fix crbug 759265 by suppressing llvm mt warnings.

[Eric Beckmann via Phabricator via llvm-commits]

ecbeckmann added a comment.

In https://reviews.llvm.org/D37240#859371, @zturner wrote:

> We have similar issues in the PDB code, where we might want to use the native pdb reader or the DIA reader.  To handle this, we have an enumeration:
>
>   /// Specifies which PDB reader implementation is to be used.  Only a value
>   /// of PDB_ReaderType::DIA is currently supported, but Native is in the works.
>   enum class PDB_ReaderType {
>     DIA = 0,
>     Native = 1,
>   };
>
>
> Then, when you actually call the function to load the PDB, it looks like this:
>
>     // Create the correct concrete instance type based on the value of Type.
>     if (Type == PDB_ReaderType::Native)
>       return NativeSession::createFromPdb(Path, Session);
>  
>   #if LLVM_ENABLE_DIA_SDK
>     return DIASession::createFromPdb(Path, Session);
>   #else
>     return make_error<GenericError>("DIA is not installed on the system");
>   #endif
>
>
> Then the user could write something like:
>
>   if (auto EC = loadDataForPDB(PDB_ReaderType::DIA, Session)) {
>     if (auto EC2 = loadDataForPDB(PDB_ReaderType::Native, Session)) {
>       return make_error<Foo>("PDB Unsupported");
>     }
>   }
>  
>   // Now I have either a native session or a DIA session, but I don't know (or care) which.
>   return Session;
>  
>


This could work, however would require copying the Executor class code or else refactoring it into a library.  Does adding a preprocessor flag work?



================
Comment at: lld/COFF/DriverUtils.cpp:431
+  std::string Msg = toString(InternalOutputBufferOrError.takeError());
+  if (Msg != "no libxml2")
+    fatal("error with internal manifest tool:" + Msg);
----------------
ruiu wrote:
> ecbeckmann wrote:
> > hans wrote:
> > > I suppose this will solve the immediate problem, but it still looks a bit strange.
> > > 
> > > I'm not very familiar with this code, but don't we know beforehand if the internal mt tool is available and expected to work?
> > > 
> > > As Rui pointed out, why can't we just do:
> > > 
> > > ```
> > > If the internal mt is present:
> > >   If merge fails for some reason:
> > >     Report that error and abort
> > > Otherwise:
> > >   Invoke external mt command, and if it fails, abort
> > > ```
> > The only way we can tell if it will work is if we see if libxml2 is enabled, but as I said before that is an implementation detail internal to the library that users should not need to know about.  I suppose we can add a cmake variable that has the same value as the libxml2 flag.
> I don't think whether a library is available or not is an internal detail of the library. It is in fact a very important information that all users of the library would want to know. Why don't you define something like `bool windows_manifest::WindowsManifestMerger::isAvailable()` which is true if it is available?
I added a preprocessor flag, is that suitable?


https://reviews.llvm.org/D37240