[PATCH] D141019: Support/CommandLine: replace argument mapping error with a warning

[Manuel Ullmann via Phabricator via llvm-commits]

labre created this revision.
labre added a reviewer: chandlerc.
labre added a project: LLVM.
Herald added subscribers: hiraditya, Anastasia.
Herald added a project: All.
labre requested review of this revision.
Herald added a subscriber: llvm-commits.

This is a “fix something while breaking other stuff” kind of change. So it probably should not be merged. I’m bringing this up as a reminder that CommandLine needs a major rework.

On to the rationale:

The comment below that change seems to indicate, that CommandLine inconsistencies are strictly unrecoverable. However this is not the case, if two libraries link dynamically to the same libLLVM version. Applying this patch results in error-free operation of these cases. This most commonly occurs in mesa, if two libraries link to libLLVM, e.g. the Clover and RocM OpenCL backends or the radeonsi VAAPI driver, LLVM aborts the application using them.

Consequently, having both Clover and RocM installed leads to a complete breakage of OpenCL device enumeration, as outlined in the Gentoo Wiki. [1] There is also the existing use case of mpv with vapoursynth support, using a plugin like SVP [2] to post process hardware decoded video. Other use cases in that context come into mind, like video editing software.

So we have valid non-breaking use cases while for now a fatal error has been reported. Changing this into a warning definitely will break other things, so this can merely serve as a temporary solution until better and more specific error handling has been fleshed out.

Since this only is reproduced by the lib linked twice or more often, I struggle to create a regression test. Python is an interpreted language, so I can not influence the linkage and I doubt, that importing LLVM twice would cause that. I’d appreciate any hints on that. On the other hand I also do not expect this to be merged like this.

Finally, I should add, that I’m unable to convince Gentoo ebuilds to link against LLVM 16.x git, so I could only test this with 15.x git. However the patch applies to both branches, so this should not matter too much.

This probably should be discussed broader in a RFC, but I’m hesitating to create one, because I can’t really think of solutions, mostly, because I’m neither familiar with C++ nor LLVM. As an end user I can just suggest two things for the rework:

1. (Somehow) detect and print clearly, when a LLVM version mismatch occurs.
2. Print the library names linking against libLLVM.

This would greatly simplify handling these issues for distributors (and it would spared me from spending 3 days until I found the root cause), but it most likely does not solve the underlying problem.

[1]: https://wiki.gentoo.org/wiki/OpenCL#AMD
[2]: https://www.svp-team.com/wiki/SVP:Linux
Related bugs:
[3]: https://github.com/llvm/llvm-project/issues/23326
[4]: https://github.com/llvm/llvm-project/issues/29935


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D141019

Files:
  llvm/lib/Support/CommandLine.cpp


Index: llvm/lib/Support/CommandLine.cpp
===================================================================
--- llvm/lib/Support/CommandLine.cpp
+++ llvm/lib/Support/CommandLine.cpp
@@ -43,6 +43,7 @@
 #include "llvm/Support/Process.h"
 #include "llvm/Support/StringSaver.h"
 #include "llvm/Support/VirtualFileSystem.h"
+#include "llvm/Support/WithColor.h"
 #include "llvm/Support/raw_ostream.h"
 #include <cstdlib>
 #include <string>
@@ -216,9 +217,8 @@

       // Add argument to the argument map!
       if (!SC->OptionsMap.insert(std::make_pair(O->ArgStr, O)).second) {
-        errs() << ProgramName << ": CommandLine Error: Option '" << O->ArgStr
+       WithColor::warning() << ProgramName << ": CommandLine Error: Option '" << O->ArgStr
                << "' registered more than once!\n";
-        HadErrors = true;
       }
     }

@@ -235,10 +235,14 @@
       SC->ConsumeAfterOpt = O;
     }

-    // Fail hard if there were errors. These are strictly unrecoverable and
-    // indicate serious issues such as conflicting option names or an
-    // incorrectly
-    // linked LLVM distribution.
+    // Fail hard if there were errors. These are strictly
+    // unrecoverable and indicate serious issues such as conflicting
+    // option names or an incorrectly linked LLVM distribution.
+    //
+    // Argument mapping errors may indicate issues, but also indicate
+    // two libraries linking dynamically to the same libLLVM, which
+    // has valid use cases, e.g. simultaneously using OpenCL and
+    // VAAPI.  Therefore it is only warned about that.
     if (HadErrors)
       report_fatal_error("inconsistency in registered CommandLine options");



-------------- next part --------------
A non-text attachment was scrubbed...
Name: D141019.486342.patch
Type: text/x-patch
Size: 1668 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230105/0fe5d30a/attachment.bin>