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

[Eric Beckmann via Phabricator via llvm-commits]

ecbeckmann added inline comments.


================
Comment at: lld/COFF/DriverUtils.cpp:435
+  if (ExternalOutputBufferOrError) {
+    // Supress internal mt errors when manifest merge succeeds.
+    consumeError(InternalOutputBufferOrError.takeError());
----------------
ruiu wrote:
> zturner wrote:
> > ecbeckmann wrote:
> > > hans wrote:
> > > > Can there be other errors than not having libxml2 available? We probably don't want to suppress those.
> > > > 
> > > > Can't we tell by some ifdef or something when libxml2 is not available, and simply not try to use the internal tool then?
> > > No I believe we still do.  The end developer will probably not care much about these errors if the manifest file is still merged in the end.  We only really want to display them if the manifest file can't be merged at all, so we want to give a list of what went wrong.
> > > 
> > > Using an ifdef was considered during the integration but this was decided against.  The reason is that it is an implementation detail of the internal library that it uses libxml2.  Any code that relies on the library should not have to know about libxml2, so therefore should not need to check if libxml2 is enabled before including it.
> > What kind of errors are we talking about where mt could generate errors but manifest merge would still succeed?
> I believe what we want to do is this:
> 
>   If the internal mt is present due to lack of libxml:
>     If merge fails for some reason:
>       Report that error and abort
>   Otherwise:
>     Invoke external mt command, and if it fails, abort
> 
> 
I was talking about cases where the internal mt lib could fail, but external mt.exe would succeed.  And in this case we can just silently fail because all the user cares about is that the manifests are merged.

I guess this is probably better though.  We probably want to be catching bugs with our internal library, so errors should be reported if libxml2 exists.


https://reviews.llvm.org/D37240