Skip to content

[lldb][test] Turn std::atomic libcxx test generic #146843

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jul 3, 2025

Conversation

Michael137
Copy link
Member

Split out from #146740

@Michael137 Michael137 requested a review from JDevlieghere as a code owner July 3, 2025 10:07
@llvmbot llvmbot added the lldb label Jul 3, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 3, 2025

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

Split out from #146740


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

3 Files Affected:

  • (renamed) lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/atomic/Makefile (-2)
  • (renamed) lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/atomic/TestStdAtomic.py (+15-8)
  • (renamed) lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/atomic/main.cpp ()
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/atomic/Makefile b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/atomic/Makefile
similarity index 51%
rename from lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/atomic/Makefile
rename to lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/atomic/Makefile
index b016f006747da..99998b20bcb05 100644
--- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/atomic/Makefile
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/atomic/Makefile
@@ -1,5 +1,3 @@
 CXX_SOURCES := main.cpp
-CXXFLAGS_EXTRAS := -std=c++11
-USE_LIBCPP := 1
 
 include Makefile.rules
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/atomic/TestLibCxxAtomic.py b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/atomic/TestStdAtomic.py
similarity index 84%
rename from lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/atomic/TestLibCxxAtomic.py
rename to lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/atomic/TestStdAtomic.py
index c6592ede03147..b0df8a525d280 100644
--- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/atomic/TestLibCxxAtomic.py
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/atomic/TestStdAtomic.py
@@ -9,18 +9,15 @@
 from lldbsuite.test import lldbutil
 
 
-class LibCxxAtomicTestCase(TestBase):
+class StdAtomicTestCase(TestBase):
     def get_variable(self, name):
         var = self.frame().FindVariable(name)
         var.SetPreferDynamicValue(lldb.eDynamicCanRunTarget)
         var.SetPreferSyntheticValue(True)
         return var
 
-    @skipIf(compiler=["gcc"])
-    @add_test_categories(["libc++"])
-    def test(self):
-        """Test that std::atomic as defined by libc++ is correctly printed by LLDB"""
-        self.build()
+    def do_test(self):
+        """Test that std::atomic is correctly printed by LLDB"""
         self.runCmd("file " + self.getBuildArtifact("a.out"), CURRENT_EXECUTABLE_SET)
 
         bkpt = self.target().FindBreakpointByID(
@@ -31,8 +28,6 @@ def test(self):
 
         self.runCmd("run", RUN_SUCCEEDED)
 
-        lldbutil.skip_if_library_missing(self, self.target(), re.compile(r"libc\+\+"))
-
         # The stop reason of the thread should be breakpoint.
         self.expect(
             "thread list",
@@ -66,3 +61,15 @@ def test(self):
         self.expect(
             "frame var p.child.parent", substrs=["p.child.parent = {\n  Value = 0x"]
         )
+
+    @skipIf(compiler=["gcc"])
+    @add_test_categories(["libc++"])
+    def test_libcxx(self):
+        self.build(dictionary={"USE_LIBCPP" : 1})
+        self.do_test()
+
+    @skipIf(compiler=["gcc"])
+    @add_test_categories(["libstdcxx"])
+    def test_libstdcxx(self):
+        self.build(dictionary={"USE_LIBSTDCPP" : 1})
+        self.do_test()
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/atomic/main.cpp b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/atomic/main.cpp
similarity index 100%
rename from lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx/atomic/main.cpp
rename to lldb/test/API/functionalities/data-formatter/data-formatter-stl/generic/atomic/main.cpp

Copy link

github-actions bot commented Jul 3, 2025

✅ With the latest revision this PR passed the Python code formatter.

Copy link

github-actions bot commented Jul 3, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@labath
Copy link
Collaborator

labath commented Jul 3, 2025

The libstdc++ test fails. Maybe we don't even have a formatter for std::atomic there? It currently looks like:

(std::atomic<int>) i = {
  std::__atomic_base<int> = {
    _M_i = 47
  }
}

.. which isn't that bad. Much better than libc++'s

(std::__1::atomic<int>) i = {
  std::__1::__atomic_base<int, true> = {
    std::__1::__atomic_base<int, false> = {
      __a_ = {
        std::__1::__cxx_atomic_base_impl<int> = {
          __a_value = 47
        }
      }
    }
  }
}

We may still want to have a data formatter for consistency, but I suspect that's not what you're looking to do now. Maybe just move this part into the generic folder, but don't add the libstdc++ test case yet?

@labath
Copy link
Collaborator

labath commented Jul 3, 2025

Btw, I've recently learned that synthetic child providers don't have to provide just children. They can also provide "values" of the object (get_value in python, I don't know how it's surfaced in c++). std::atomic would be a prime candidate for that.. just in case you're interested :)

I think it would turn

(std::atomic<int>) i = {
  Value = 47
}

into

(std::atomic<int>) i = 47

@Michael137
Copy link
Member Author

The libstdc++ test fails. Maybe we don't even have a formatter for std::atomic there? It currently looks like:


(std::atomic<int>) i = {

  std::__atomic_base<int> = {

    _M_i = 47

  }

}

.. which isn't that bad. Much better than libc++'s


(std::__1::atomic<int>) i = {

  std::__1::__atomic_base<int, true> = {

    std::__1::__atomic_base<int, false> = {

      __a_ = {

        std::__1::__cxx_atomic_base_impl<int> = {

          __a_value = 47

        }

      }

    }

  }

}

We may still want to have a data formatter for consistency, but I suspect that's not what you're looking to do now. Maybe just move this part into the generic folder, but don't add the libstdc++ test case yet?

Huh I'm surprised that's what it looks like for libc++ tbh. Not sure what our formatter is doing.

For libstdc++ we dont have a formatter. I think that's the case for a decent number of our libcxx formatters. My plan was to just XFAIL. But just omitting the libstdc++ category entirely for the unsupported formatters seems better

@Michael137
Copy link
Member Author

Michael137 commented Jul 3, 2025

Heh looks like the LibCxxAtomic formatter doesn't support post-cxx03 layouts. Lets fix that separately

@labath
Copy link
Collaborator

labath commented Jul 3, 2025

Huh I'm surprised that's what it looks like for libc++ tbh. Not sure what our formatter is doing.

No, that was the raw output. I was just putting it there to show that the libstdc++ output (where we only have the raw output) isn't that bad, which reduces the motivation to have the formatter in the first place.

@Michael137
Copy link
Member Author

Huh I'm surprised that's what it looks like for libc++ tbh. Not sure what our formatter is doing.

No, that was the raw output. I was just putting it there to show that the libstdc++ output (where we only have the raw output) isn't that bad, which reduces the motivation to have the formatter in the first place.

Ahh gotcha. Makes sense

@Michael137 Michael137 merged commit b0444b0 into llvm:main Jul 3, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants