Skip to content
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

[lldb][test] TestDataFormatterLibcxxStringSimulator.py: add new padding layout #108375

Merged

Conversation

Michael137
Copy link
Member

Depends on #108362 and #108343.

Adds new layout for #105865.

@llvmbot
Copy link
Member

llvmbot commented Sep 12, 2024

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

Depends on #108362 and #108343.

Adds new layout for #105865.


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

2 Files Affected:

  • (modified) lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/string/TestDataFormatterLibcxxStringSimulator.py (+4-1)
  • (modified) lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/string/main.cpp (+33-10)
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/string/TestDataFormatterLibcxxStringSimulator.py b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/string/TestDataFormatterLibcxxStringSimulator.py
index 3e5c493692c4f0..dd9f5de22c9057 100644
--- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/string/TestDataFormatterLibcxxStringSimulator.py
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/string/TestDataFormatterLibcxxStringSimulator.py
@@ -22,9 +22,12 @@ def _run_test(self, defines):
         self.expect_var_path("shortstring", summary='"short"')
         self.expect_var_path("longstring", summary='"I am a very long string"')
 
+        self.expect_expr("shortstring", result_summary='"short"')
+        self.expect_expr("longstring", result_summary='"I am a very long string"')
+
 
 for v in [None, "ALTERNATE_LAYOUT"]:
-    for r in range(5):
+    for r in range(6):
         name = "test_r%d" % r
         defines = ["REVISION=%d" % r]
         if v:
diff --git a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/string/main.cpp b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/string/main.cpp
index 7beeb9c39de49e..5cf3a85007fb64 100644
--- a/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/string/main.cpp
+++ b/lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/string/main.cpp
@@ -20,7 +20,11 @@
 // Pre-D128285 layout.
 #define PACKED_ANON_STRUCT
 #endif
-// REVISION == 4: current layout
+#if REVISION <= 4
+// Pre-TODO layout.
+#define UB_PADDING
+#endif
+// REVISION == 5: current layout
 
 #ifdef PACKED_ANON_STRUCT
 #define BEGIN_PACKED_ANON_STRUCT struct __attribute__((packed)) {
@@ -34,6 +38,7 @@
 namespace std {
 namespace __lldb {
 
+#ifdef UB_PADDING
 #if defined(ALTERNATE_LAYOUT) && defined(SUBCLASS_PADDING)
 template <class _CharT, size_t = sizeof(_CharT)> struct __padding {
   unsigned char __xx[sizeof(_CharT) - 1];
@@ -41,6 +46,13 @@ template <class _CharT, size_t = sizeof(_CharT)> struct __padding {
 
 template <class _CharT> struct __padding<_CharT, 1> {};
 #endif
+#else // !UB_PADDING
+template <size_t _PaddingSize> struct __padding {
+  char __padding_[_PaddingSize];
+};
+
+template <> struct __padding<0> {};
+#endif
 
 template <class _CharT, class _Traits, class _Allocator> class basic_string {
 public:
@@ -71,19 +83,25 @@ template <class _CharT, class _Traits, class _Allocator> class basic_string {
 
   struct __short {
     value_type __data_[__min_cap];
-#ifdef BITMASKS
 #ifdef SUBCLASS_PADDING
     struct : __padding<value_type> {
       unsigned char __size_;
     };
-#else
+#else // !SUBCLASS_PADDING
+
+#ifdef UB_PADDING
     unsigned char __padding[sizeof(value_type) - 1];
-    unsigned char __size_;
+#else
+    [[no_unique_address]] __padding<sizeof(value_type) - 1> __padding_;
 #endif
+
+#ifdef BITMASKS
+    unsigned char __size_;
 #else // !BITMASKS
     unsigned char __size_ : 7;
     unsigned char __is_long_ : 1;
-#endif
+#endif // BITMASKS
+#endif // SUBCLASS_PADDING
   };
 
 #ifdef BITMASKS
@@ -128,21 +146,26 @@ template <class _CharT, class _Traits, class _Allocator> class basic_string {
     union {
 #ifdef BITMASKS
       unsigned char __size_;
-#else
+#else  // !BITMASKS
       struct {
         unsigned char __is_long_ : 1;
         unsigned char __size_ : 7;
       };
-#endif
+#endif // BITMASKS
       value_type __lx;
     };
-#else
+#else  // !SHORT_UNION
     BEGIN_PACKED_ANON_STRUCT
     unsigned char __is_long_ : 1;
     unsigned char __size_ : 7;
     END_PACKED_ANON_STRUCT
-    char __padding_[sizeof(value_type) - 1];
-#endif
+#ifdef UB_PADDING
+    unsigned char __padding[sizeof(value_type) - 1];
+#else  // !UB_PADDING
+    [[no_unique_address]] __padding<sizeof(value_type) - 1> __padding_;
+#endif // UB_PADDING
+
+#endif // SHORT_UNION
     value_type __data_[__min_cap];
   };
 

@Michael137 Michael137 force-pushed the lldb/simulate-new-string-padding-layout branch from 4e4f1ea to 35971a4 Compare September 12, 2024 16:27
@@ -77,7 +89,12 @@ template <class _CharT, class _Traits, class _Allocator> class basic_string {
};
#else // !SUBCLASS_PADDING

#ifdef UB_PADDING
unsigned char __padding[sizeof(value_type) - 1];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was that (the zero sized array) really UB? Or just a compiler extension?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I'll rename it to NON_STANDARD_PADDING

@Michael137 Michael137 force-pushed the lldb/simulate-new-string-padding-layout branch from 35971a4 to d3d78e4 Compare September 13, 2024 10:17
@Michael137 Michael137 force-pushed the lldb/simulate-new-string-padding-layout branch from d3d78e4 to 6706dae Compare October 3, 2024 11:24
@Michael137
Copy link
Member Author

(this libc++-side change was relanded in: #108867)

@Michael137 Michael137 merged commit d5f6e88 into llvm:main Oct 3, 2024
7 checks passed
@Michael137 Michael137 deleted the lldb/simulate-new-string-padding-layout branch October 3, 2024 11:53
@Michael137
Copy link
Member Author

Broke the lldb-aarch64-windows2921 bot

Michael137 added a commit that referenced this pull request Oct 3, 2024
…ew padding layout (#108375)"

This reverts commit d5f6e88.

Caused failure on Windows CI. Following test failed:
```
Config=aarch64-C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\bin\clang.exe

======================================================================

FAIL: test_r5_c2_ALTERNATE_LAYOUT (TestDataFormatterLibcxxStringSimulator.LibcxxStringDataFormatterSimulatorTestCase.test_r5_c2_ALTERNATE_LAYOUT)

    partial(func, *args, **keywords) - new function with partial application

----------------------------------------------------------------------

Traceback (most recent call last):

  File "C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\test\API\functionalities\data-formatter\data-formatter-stl\libcxx-simulators\string\TestDataFormatterLibcxxStringSimulator.py", line 23, in _run_test

    self.expect_var_path("longstring", summary='"I am a very long string"')

  File "C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\packages\Python\lldbsuite\test\lldbtest.py", line 2552, in expect_var_path

    value_check.check_value(self, eval_result, str(eval_result))

  File "C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\packages\Python\lldbsuite\test\lldbtest.py", line 321, in check_value

    test_base.assertEqual(

AssertionError: '"I am a very long string"' != '""'

- "I am a very long string"

+ ""

 : (std::__lldb::string) longstring = ""

Checking SBValue: (std::__lldb::string) longstring = ""
```

We may need to use `msvc::no_unique_address` around the simulators.
Michael137 added a commit to Michael137/llvm-project that referenced this pull request Oct 4, 2024
Michael137 added a commit to Michael137/llvm-project that referenced this pull request Oct 4, 2024
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
xgupta pushed a commit to xgupta/llvm-project that referenced this pull request Oct 4, 2024
…ew padding layout (llvm#108375)"

This reverts commit d5f6e88.

Caused failure on Windows CI. Following test failed:
```
Config=aarch64-C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\build\bin\clang.exe

======================================================================

FAIL: test_r5_c2_ALTERNATE_LAYOUT (TestDataFormatterLibcxxStringSimulator.LibcxxStringDataFormatterSimulatorTestCase.test_r5_c2_ALTERNATE_LAYOUT)

    partial(func, *args, **keywords) - new function with partial application

----------------------------------------------------------------------

Traceback (most recent call last):

  File "C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\test\API\functionalities\data-formatter\data-formatter-stl\libcxx-simulators\string\TestDataFormatterLibcxxStringSimulator.py", line 23, in _run_test

    self.expect_var_path("longstring", summary='"I am a very long string"')

  File "C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\packages\Python\lldbsuite\test\lldbtest.py", line 2552, in expect_var_path

    value_check.check_value(self, eval_result, str(eval_result))

  File "C:\Users\tcwg\llvm-worker\lldb-aarch64-windows\llvm-project\lldb\packages\Python\lldbsuite\test\lldbtest.py", line 321, in check_value

    test_base.assertEqual(

AssertionError: '"I am a very long string"' != '""'

- "I am a very long string"

+ ""

 : (std::__lldb::string) longstring = ""

Checking SBValue: (std::__lldb::string) longstring = ""
```

We may need to use `msvc::no_unique_address` around the simulators.
Michael137 added a commit that referenced this pull request Oct 7, 2024
…ew padding layout" (#111123)

Relands #108375 which had to be
reverted because it was failing on the Windows buildbot. Trying to
reland this with `msvc::no_unique_address` on Windows.
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