[clang] [unittest] Add a test for Generic_GCC::GCCVersion::Parse

[mstorsjo]

This adds actual test cases for all the cases that are listed in a code comment in the implementation of this function; having such test coverage eases doing further modifications to the function.

[llvmbot]

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Martin Storsjö (mstorsjo)

Changes

This adds actual test cases for all the cases that are listed in a code comment in the implementation of this function; having such test coverage eases doing further modifications to the function.

---

Full diff: https://github.com/llvm/llvm-project/pull/69078.diff

2 Files Affected:

• (modified) clang/unittests/Driver/CMakeLists.txt (+1)
• (added) clang/unittests/Driver/GCCVersionTest.cpp (+48)

diff --git a/clang/unittests/Driver/CMakeLists.txt b/clang/unittests/Driver/CMakeLists.txt
index e37c158d7137a88..752037f78fb147d 100644
--- a/clang/unittests/Driver/CMakeLists.txt
+++ b/clang/unittests/Driver/CMakeLists.txt
@@ -9,6 +9,7 @@ set(LLVM_LINK_COMPONENTS
 add_clang_unittest(ClangDriverTests
   DistroTest.cpp
   DXCModeTest.cpp
+  GCCVersionTest.cpp
   ToolChainTest.cpp
   ModuleCacheTest.cpp
   MultilibBuilderTest.cpp
diff --git a/clang/unittests/Driver/GCCVersionTest.cpp b/clang/unittests/Driver/GCCVersionTest.cpp
new file mode 100644
index 000000000000000..ef05a0b4fe734e5
--- /dev/null
+++ b/clang/unittests/Driver/GCCVersionTest.cpp
@@ -0,0 +1,48 @@
+//===- unittests/Driver/GCCVersionTest.cpp --- GCCVersion parser tests ----===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Unit tests for Generic_GCC::GCCVersion
+//
+//===----------------------------------------------------------------------===//
+
+#include "../../lib/Driver/ToolChains/Gnu.h"
+#include "gtest/gtest.h"
+
+using namespace clang::driver;
+using namespace clang;
+
+struct VersionParseTest {
+  std::string Text;
+
+  int Major, Minor, Patch;
+  std::string MajorStr, MinorStr, PatchSuffix;
+};
+
+const VersionParseTest TestCases[] = {
+    {"5", 5, -1, -1, "5", "", ""},
+    {"4.4", 4, 4, -1, "4", "4", ""},
+    {"4.4-patched", 4, 4, -1, "4", "4", "-patched"},
+    {"4.4.0", 4, 4, 0, "4", "4", ""},
+    {"4.4.x", 4, 4, -1, "4", "4", ""},
+    {"4.4.2-rc4", 4, 4, 2, "4", "4", "-rc4"},
+    {"4.4.x-patched", 4, 4, -1, "4", "4", ""},
+    {"not-a-version", -1, -1, -1, "", "", ""},
+};
+
+TEST(GCCVersionTest, Parse) {
+  for (const auto &TC : TestCases) {
+    auto V = toolchains::Generic_GCC::GCCVersion::Parse(TC.Text);
+    ASSERT_EQ(V.Text, TC.Text);
+    ASSERT_EQ(V.Major, TC.Major);
+    ASSERT_EQ(V.Minor, TC.Minor);
+    ASSERT_EQ(V.Patch, TC.Patch);
+    ASSERT_EQ(V.MajorStr, TC.MajorStr);
+    ASSERT_EQ(V.MinorStr, TC.MinorStr);
+    ASSERT_EQ(V.PatchSuffix, TC.PatchSuffix);
+  }
+}

[mstorsjo]

Thanks, I applied the suggested changes - will push in a little while.

[tbaederr]

This doesn't build for me locally:

mold: error: undefined symbol: tools/clang/unittests/Driver/CMakeFiles/ClangDriverTests.dir/GCCVersionTest.cpp.o: clang::driver::toolchains::Generic_GCC::GCCVersion::Parse(llvm::StringRef)

[asb]

@tbaederr Just came to report the same thing!

@mstorsjo This broke builds that use -DBUILD_SHARED_LIBS=True. The problem seems to be that the Generic_GCC class has the LLVM_LIBRARY_VISIBILITY attribute meaning the clang::driver::toolchains::Generic_GCC::GCCVersion::Parse(llvm::StringRef) symbol is hidden. Removing that attribute fixes the build. It would be good for someone more familiar with this part of the codebase to confirm if that's an acceptable fix however.

[mstorsjo]

@tbaederr Just came to report the same thing!

@mstorsjo This broke builds that use -DBUILD_SHARED_LIBS=True.

Thanks! That was my guess as well, I was running a build with that enabled to try to reproduce @tbaederr 's issue.

The problem seems to be that the Generic_GCC class has the LLVM_LIBRARY_VISIBILITY attribute meaning the clang::driver::toolchains::Generic_GCC::GCCVersion::Parse(llvm::StringRef) symbol is hidden. Removing that attribute fixes the build. It would be good for someone more familiar with this part of the codebase to confirm if that's an acceptable fix however.

Thanks for the analysis! I guess that sounds reasonable, although I wonder how these unit test, that test internals within libclang are meant to work in this configuration. The number of extra exported symbols by removing LLVM_LIBRARY_VISIBILITY probably isn't that bad, I wonder if it has other implications wrt ABI of the shared libs?

I guess it's safest to revert this for now, so we can figure out the best path forward here without a rush?

[jroelofs]

Perhaps this belongs in the ABI-breaking-checks build?

[mstorsjo]

Perhaps this belongs in the ABI-breaking-checks build?

Hmm, ideally I think it should be included in any build - let’s hope we don’t need to resort to that.

@tstellar @MaskRay Do either of you happen to know about this; would it be ok ABI wise to remove LLVM_LIBRARY_VISIBILITY from clang::driver::toolchains::Generic_GCC in order to be able to access it from unit tests in shared or dylib builds?

[MaskRay]

I hope that we do not drop LLVM_LIBRARY_VISIBILITY arbitrarily from clang::driver::toolchains::* classes, just because some unittests need to reference the symbols in a shared object.

#if !defined(LLVM_BUILD_SHARED_LIBS)

is not great but is not too bad. -DBUILD_SHARED_LIBS=on modes are slow to execute tests and are not used often for Release builds.

[mstorsjo]

I hope that we do not drop LLVM_LIBRARY_VISIBILITY arbitrarily from clang::driver::toolchains::* classes, just because some unittests need to reference the symbols in a shared object.

That’s a reasonable point.

#if !defined(LLVM_BUILD_SHARED_LIBS)

is not great but is not too bad. -DBUILD_SHARED_LIBS=on modes are slow to execute tests and are not used often for Release builds.

I guess that’s a reasonable tradeoff. We’d need to do the same for dylib mode as well, which probably is used a bit more. But the main point is that whoever is working on modifying that implementation can run the tests fairly easy, and that they get run somewhere in some configurations at least.

[tbaederr]

FWIW this failed for me locally as well but I'm not using BUILD_SHARED_LIBS. I am linking with the clang dylib though, which has the same problem I think.

[mstorsjo]

#if !defined(LLVM_BUILD_SHARED_LIBS)

is not great but is not too bad. -DBUILD_SHARED_LIBS=on modes are slow to execute tests and are not used often for Release builds.

I went ahead and relanded this now, in 538b7ba, with #if !defined(LLVM_BUILD_LLVM_DYLIB) && !defined(LLVM_BUILD_SHARED_LIBS). Not great, but probably the least bad compromise.