Skip to content

[lldb][test] TestDataFormatterLibcxxStringSimulator.py: fix padding for current layout #108362

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

Conversation

Michael137
Copy link
Member

@Michael137 Michael137 commented Sep 12, 2024

IIUC, the history of std::string's __short structure in the alternate ABI layout (as recorded by the simulator test) looks as follows:

  • First layout ( SUBCLASS_PADDING is defined):
     struct __short                                          
     {                                                       
         value_type __data_[__min_cap];                      
        struct                                              
            : __padding<value_type>                         
        {                                                   
            unsigned char __size_;                          
        };
     };                                      
  • Then:
     struct __short                                         
     {                                                      
        value_type __data_[__min_cap];                                                               
        unsigned char __padding[sizeof(value_type) - 1];   
        unsigned char __size_;                             
     };
  • Then, post-BITMASKS:
     struct __short                                         
     {                                                      
        value_type __data_[__min_cap];                                                               
        unsigned char __padding[sizeof(value_type) - 1];   
        unsigned char __size_ : 7;
        unsigned char __is_long_ : 1;                             
     };

Which is the one that's on top-of-tree.

But for REVISION > 1, BITMASKS is never set, so for those tests we lose the __padding member.

This patch fixes this by splitting out the SUBCLASS_PADDING out of the ifdef.

Drive-by:

  • Also run expression evaluator on the string to provide is with some extra coverage.

…or current layout

Drive-by:
* Also run expression evaluator on the string to
  provide is with some extra coverage.
@llvmbot
Copy link
Member

llvmbot commented Sep 12, 2024

@llvm/pr-subscribers-lldb

Author: Michael Buch (Michael137)

Changes

IIUC, the history of std::string's __short structure in the alternate ABI layout (as recorded by the simulator test) looks as follows:

  • First layout ( SUBCLASS_PADDING is defined):
     struct __short                                          
     {                                                       
         value_type __data_[__min_cap];                      
        struct                                              
            : __padding&lt;value_type&gt;                         
        {                                                   
            unsigned char __size_;                          
        };
     };                                      
  • Then:
     struct __short                                         
     {                                                      
        value_type __data_[__min_cap];                                                               
        unsigned char __padding[sizeof(value_type) - 1];   
        unsigned char __size_;                             
     };
  • Then, post-BITMASKS:
     struct __short                                         
     {                                                      
        value_type __data_[__min_cap];                                                               
        unsigned char __padding[sizeof(value_type) - 1];   
        unsigned char __size_ : 7;
        unsigned char __is_long_ : 1;                             
     };

Which is the one that's on top-of-tree.

But for REVISION &gt; 2, BITMASKS is never set, so for those tests we lose the __padding member.

This patch fixes this by

Drive-by:

  • Also run expression evaluator on the string to provide is with some extra coverage.

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

2 Files Affected:

  • (modified) lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/string/TestDataFormatterLibcxxStringSimulator.py (+3)
  • (modified) lldb/test/API/functionalities/data-formatter/data-formatter-stl/libcxx-simulators/string/main.cpp (+3-3)
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..98d2c7320003e4 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,6 +22,9 @@ 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):
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..74baeba6ec879b 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
@@ -71,15 +71,15 @@ 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
+
     unsigned char __padding[sizeof(value_type) - 1];
+#ifdef BITMASKS
     unsigned char __size_;
-#endif
 #else // !BITMASKS
     unsigned char __size_ : 7;
     unsigned char __is_long_ : 1;

Michael137 added a commit to Michael137/llvm-project that referenced this pull request Sep 12, 2024
Michael137 added a commit to Michael137/llvm-project that referenced this pull request Sep 12, 2024
@Michael137 Michael137 merged commit 4ca8fb1 into llvm:main Sep 13, 2024
7 checks passed
@Michael137 Michael137 deleted the lldb/simulator-string-padding-layout-fix branch September 13, 2024 10:01
Michael137 added a commit to Michael137/llvm-project that referenced this pull request Sep 13, 2024
Michael137 added a commit to Michael137/llvm-project that referenced this pull request Oct 3, 2024
Michael137 added a commit that referenced this pull request Oct 3, 2024
Michael137 added a commit to swiftlang/llvm-project that referenced this pull request Apr 17, 2025
…or current layout (llvm#108362)

IIUC, the history of `std::string`'s `__short` structure in the
alternate ABI layout (as recorded by the simulator test) looks as
follows:
* First layout ( `SUBCLASS_PADDING` is defined):
```
     struct __short
     {
         value_type __data_[__min_cap];
        struct
            : __padding<value_type>
        {
            unsigned char __size_;
        };
     };
```
* Then:
```
     struct __short
     {
        value_type __data_[__min_cap];
        unsigned char __padding[sizeof(value_type) - 1];
        unsigned char __size_;
     };
```
* Then, post-`BITMASKS`:
```
     struct __short
     {
        value_type __data_[__min_cap];
        unsigned char __padding[sizeof(value_type) - 1];
        unsigned char __size_ : 7;
        unsigned char __is_long_ : 1;
     };
```

Which is the one that's [on
top-of-tree](https://github.com/llvm/llvm-project/blob/89c10e27d8b4d5f44998aad9abd2590d9f96c5df/libcxx/include/string#L854-L859).

But for `REVISION > 1`, `BITMASKS` is never set, so for those tests we
lose the `__padding` member.

This patch fixes this by splitting out the `SUBCLASS_PADDING` out of the
ifdef.

Drive-by:
* Also run expression evaluator on the string to provide is with some
extra coverage.

(cherry picked from commit 4ca8fb1)
Michael137 added a commit to swiftlang/llvm-project that referenced this pull request Apr 17, 2025
…ng layout (llvm#108375)

Depends on llvm#108362 and
llvm#108343.

Adds new layout for llvm#105865.

(cherry picked from commit d5f6e88)
Michael137 added a commit to swiftlang/llvm-project that referenced this pull request Apr 17, 2025
…or current layout (llvm#108362)

IIUC, the history of `std::string`'s `__short` structure in the
alternate ABI layout (as recorded by the simulator test) looks as
follows:
* First layout ( `SUBCLASS_PADDING` is defined):
```
     struct __short
     {
         value_type __data_[__min_cap];
        struct
            : __padding<value_type>
        {
            unsigned char __size_;
        };
     };
```
* Then:
```
     struct __short
     {
        value_type __data_[__min_cap];
        unsigned char __padding[sizeof(value_type) - 1];
        unsigned char __size_;
     };
```
* Then, post-`BITMASKS`:
```
     struct __short
     {
        value_type __data_[__min_cap];
        unsigned char __padding[sizeof(value_type) - 1];
        unsigned char __size_ : 7;
        unsigned char __is_long_ : 1;
     };
```

Which is the one that's [on
top-of-tree](https://github.com/llvm/llvm-project/blob/89c10e27d8b4d5f44998aad9abd2590d9f96c5df/libcxx/include/string#L854-L859).

But for `REVISION > 1`, `BITMASKS` is never set, so for those tests we
lose the `__padding` member.

This patch fixes this by splitting out the `SUBCLASS_PADDING` out of the
ifdef.

Drive-by:
* Also run expression evaluator on the string to provide is with some
extra coverage.

(cherry picked from commit 4ca8fb1)
Michael137 added a commit to swiftlang/llvm-project that referenced this pull request Apr 17, 2025
…ng layout (llvm#108375)

Depends on llvm#108362 and
llvm#108343.

Adds new layout for llvm#105865.

(cherry picked from commit d5f6e88)
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