[SPIR-V] Fix parsing of command line options for the SPIR-V Backend API call #124653
Conversation
|
@llvm/pr-subscribers-backend-spir-v Author: Vyacheslav Levytskyy (VyacheslavLevytskyy) ChangesThis PR fixes parsing of command line options for the SPIR-V Backend API call. The issue was discovered in #123733 (see #123733 (comment)). Full diff: https://github.com/llvm/llvm-project/pull/124653.diff 2 Files Affected:
diff --git a/llvm/lib/Target/SPIRV/SPIRVAPI.cpp b/llvm/lib/Target/SPIRV/SPIRVAPI.cpp
index 95c9b0e5200608..6147198ce7e937 100644
--- a/llvm/lib/Target/SPIRV/SPIRVAPI.cpp
+++ b/llvm/lib/Target/SPIRV/SPIRVAPI.cpp
@@ -47,12 +47,22 @@ static cl::opt<char> SpirvOptLevel("spirv-O", cl::Hidden, cl::Prefix,
static cl::opt<std::string> SpirvTargetTriple("spirv-mtriple", cl::Hidden,
cl::init(""));
+std::once_flag InitOnceOpts;
// Utility to accept options in a command line style.
void parseSPIRVCommandLineOptions(const std::vector<std::string> &Options,
raw_ostream *Errs) {
static constexpr const char *Origin = "SPIRVTranslateModule";
+ // Initialize command line parser dependencies just once and in a
+ // thread-safe manner.
+ std::call_once(InitOnceOpts, []() {
+ std::vector<const char *> Argv(1, Origin);
+ cl::ParseCommandLineOptions(Argv.size(), Argv.data(), Origin,
+ &llvm::nulls());
+ });
+ cl::ResetAllOptionOccurrences();
if (!Options.empty()) {
std::vector<const char *> Argv(1, Origin);
+ // Parse options.
for (const auto &Arg : Options)
Argv.push_back(Arg.c_str());
cl::ParseCommandLineOptions(Argv.size(), Argv.data(), Origin, Errs);
diff --git a/llvm/unittests/Target/SPIRV/SPIRVAPITest.cpp b/llvm/unittests/Target/SPIRV/SPIRVAPITest.cpp
index f0b4a2f55c1519..60458f86521328 100644
--- a/llvm/unittests/Target/SPIRV/SPIRVAPITest.cpp
+++ b/llvm/unittests/Target/SPIRV/SPIRVAPITest.cpp
@@ -92,18 +92,29 @@ TEST_F(SPIRVAPITest, checkTranslateOk) {
}
TEST_F(SPIRVAPITest, checkTranslateError) {
- std::string Result, Error;
- bool Status = toSpirv(OkAssembly, Result, Error, {},
- {"-mtriple=spirv32-unknown-unknown"});
- EXPECT_FALSE(Status);
- EXPECT_TRUE(Result.empty());
- EXPECT_THAT(Error,
- StartsWith("SPIRVTranslateModule: Unknown command line argument "
- "'-mtriple=spirv32-unknown-unknown'"));
- Status = toSpirv(OkAssembly, Result, Error, {}, {"--spirv-O 5"});
- EXPECT_FALSE(Status);
- EXPECT_TRUE(Result.empty());
- EXPECT_EQ(Error, "Invalid optimization level!");
+ {
+ std::string Result, Error;
+ bool Status = toSpirv(OkAssembly, Result, Error, {},
+ {"-mtriple=spirv32-unknown-unknown"});
+ EXPECT_FALSE(Status);
+ EXPECT_TRUE(Result.empty());
+ EXPECT_THAT(
+ Error, StartsWith("SPIRVTranslateModule: Unknown command line argument "
+ "'-mtriple=spirv32-unknown-unknown'"));
+ }
+ {
+ std::string Result, Error;
+ bool Status = toSpirv(OkAssembly, Result, Error, {}, {"--spirv-O 5"});
+ EXPECT_FALSE(Status);
+ EXPECT_TRUE(Result.empty());
+ EXPECT_EQ(Error, "Invalid optimization level!");
+ }
+ {
+ std::string Result, Error;
+ bool Status = toSpirv(OkAssembly, Result, Error, {}, {});
+ EXPECT_TRUE(Status && Error.empty() && !Result.empty());
+ EXPECT_EQ(identify_magic(Result), file_magic::spirv_object);
+ }
}
TEST_F(SPIRVAPITest, checkTranslateSupportExtensionByOpts) {
|
|
@michalpaszkowski @nikic It's a temporary fix, in a sense that I should expect it immediately resolves the problem with builds. |
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
VyacheslavLevytskyy
force-pushed
the
fix_api_parse_cmd_opts_thread_safe
branch
from
898f393 to
7177bdf
Compare
|
buildkite fails are not related to the PR |
There was a problem hiding this comment.
I don't think this really fixes the issue. If I'm reading this correctly, you still have:
- A potential thread-safety issue, because only the option parsing is behind a lock, but not the use of those options, so they may be written and accessed concurrently.
- Clobbering of global options, which is likely to break things if the process does anything else with LLVM other than calling this function.
I think to fix this you need to switch to using a different option parsing implementation that does not use the (global) cl::opt mechanism. For example, using OptTable might work. Though ideally, the API would not require option parsing at all.
Is it possible to completely remove this function for the LLVM 20 release and then reintroduce it later with a redesigned API? |
You are right about thread-safety issue with read/write, however, this PR fixes LLVM's building errors for sure, so please think about this PR as a quick fix, not a permanent solution. The solution is to avoid using cl::opt & globals as a source of user facing options indeed (#124703). I'd expect that I may create a solution for this during next couple of days, so probably there is no need for more radical solutions, if you agree with that. |
It would be nice if we can avoid this. There is a couple of products that relies on the presence of the API already. Redesign & changing of contract is not a problem for them, but to remove the API function entirely breaks things badly. I'd propose to either apply this PR as a quick remediation of random build errors if it's an urgent situation, or to wait for a couple of days until a redesigned API is available for review. What you think? |
Is it possible to replace Opts with CodeGenOptLevel and Triple arguments as a quick fix, and then follow up with whatever general API design you have in mind? |
@nikic Yes, it's a way forward indeed. I created #124745 to review this inside the SPIR-V backend team and will add you to reviewers after done if that works for you. |
|
Close in favor of #124745 |
This PR fixes parsing of command line options for the SPIR-V Backend API call. The issue was discovered in #123733 (see #123733 (comment)).