[PATCH] D37240: Fix crbug 759265 by suppressing llvm mt warnings.
Eric Beckmann via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Sep 5 10:29:27 PDT 2017
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
More information about the llvm-commits
mailing list