[lldb][InstrumentationRuntime] Make 'data' struct anonymous in order to avoid collisions with types in the debuggee

[Michael137]

Summary

The UBSAN/ASAN plugins would previously call the internal helper
structure injected into the source expression struct data { ... };

This occasionally collided with user-defined types of the same (quite
common) name. In the presence of varibale with the same name LLDB would
simply fail to run the injected InstrumentationRuntime expressions
with:

warning: cannot evaluate AddressSanitizer expression:
expression failed to parse:
error: <user expression 2>:2:5: must use 'struct' tag to refer to type 'data' in this scope
data t;

In the presence of another type called 'data', LLDB would most likely
crash with something like:

0x00000001198de614 clang::ASTNodeImporter::ImportDeclContext(clang::DeclContext*, bool) + 1088
0x00000001198de614 clang::ASTNodeImporter::ImportDeclContext(clang::DeclContext*, bool) + 1088
0x0000000119907930 clang::ASTImporter::ImportDefinition(clang::Decl*) + 596
0x00000001163db928 lldb_private::ClangASTImporter::ASTImporterDelegate::ImportDefinitionTo(clang::Decl*, clang::Decl*) + 100
0x00000001163da070 (anonymous namespace)::CompleteTagDeclsScope::~CompleteTagDeclsScope() + 572
...

...because it got the types confused.

This patch makes these structures anonymous so there's no
chance of clashing with other types in the program. This is
already the approach taken in UBSan/InstrumentationRuntimeABSan.cpp.

Testing

• API tests still pass
• Tested manually that the memory history command now works in the presence of a local called data

[Michael137]

Not sure there's a test to be added here.

Of course I could add a dummy data type to the API tests called struct data {}; and confirm that it doesn't crash (I did this manually locally), but it doesn't seem like something that would catch regressions in the future. But if people want I can add them in

[Michael137]

• Update commit message

[jingham]

The TSAN instrumenter has the same struct data definition. I actually got a report of this crashing in TSAN yesterday. You should do that one as well. Otherwise, LGTM and I don't think you need to add a test with "struct data" in it on the off chance that someone reverts this back to exactly struct data...

[Michael137]

• Apply fix to TSAN structure as well

[Michael137]

In D145569#4179648, @jingham wrote:

The TSAN instrumenter has the same struct data definition. I actually got a report of this crashing in TSAN yesterday. You should do that one as well. Otherwise, LGTM and I don't think you need to add a test with "struct data" in it on the off chance that someone reverts this back to exactly struct data...

Ah good point, done

[jingham]

LGTM

lldb/source/Plugins/InstrumentationRuntime/TSan/InstrumentationRuntimeTSan.cpp

int __tsan_get_report_thread(void *report, unsigned long idx, int *tid, unsigned long *os_id,
                                  int *running, const char **name, int *parent_tid,
                                  void **trace, unsigned long trace_size);
     int __tsan_get_report_unique_tid(void *report, unsigned long idx, int *tid);
 
     // TODO: dlsym won't work on Windows.
     void *dlsym(void* handle, const char* symbol);
     int (*ptr__tsan_get_report_loc_object_type)(void *report, unsigned long idx, const char **object_type);
 }
+)";
+
+const char *thread_sanitizer_retrieve_report_data_command = R"(
 
 const int REPORT_TRACE_SIZE = 128;
 const int REPORT_ARRAY_SIZE = 4;
 
 struct data {
     void *report;
     const char *description;
     int report_count;
struct {
         void *trace[REPORT_TRACE_SIZE];
     } threads[REPORT_ARRAY_SIZE];
 
     int unique_tid_count;
     struct {
         int idx;
         int tid;
     } unique_tids[REPORT_ARRAY_SIZE];
-};
-)";
-
-const char *thread_sanitizer_retrieve_report_data_command = R"(
-data t = {0};
+} t = {0};
 
 ptr__tsan_get_report_loc_object_type = (typeof(ptr__tsan_get_report_loc_object_type))(void *)dlsym((void*)-2 /*RTLD_DEFAULT*/, "__tsan_get_report_loc_object_type");
 
 t.report = __tsan_get_current_report();
 __tsan_get_report_data(t.report, &t.description, &t.report_count, &t.stack_count, &t.mop_count, &t.loc_count, &t.mutex_count, &t.thread_count, &t.unique_tid_count, t.sleep_trace, REPORT_TRACE_SIZE);
 
 if (t.stack_count > REPORT_ARRAY_SIZE) t.stack_count = REPORT_ARRAY_SIZE;
 for (int i = 0; i < t.stack_count; i++) {

lldb/source/Plugins/InstrumentationRuntime/UBSan/InstrumentationRuntimeUBSan.cpp

 
 static const char *ub_sanitizer_retrieve_report_data_prefix = R"(
 extern "C" {
 void
 __ubsan_get_current_report_data(const char **OutIssueKind,
     const char **OutMessage, const char **OutFilename, unsigned *OutLine,
     unsigned *OutCol, char **OutMemoryAddr);
 }
+)";
 
-struct data {
+static const char *ub_sanitizer_retrieve_report_data_command = R"(
+struct {
   const char *issue_kind;
   const char *message;
   const char *filename;
   unsigned line;
   unsigned col;
   char *memory_addr;
-};
-)";
+} t;
 
-static const char *ub_sanitizer_retrieve_report_data_command = R"(
-data t;
 __ubsan_get_current_report_data(&t.issue_kind, &t.message, &t.filename, &t.line,
                                 &t.col, &t.memory_addr);
 t;
 )";
 
 static addr_t RetrieveUnsigned(ValueObjectSP return_value_sp,
                                ProcessSP process_sp,
                                const std::string &expression_path) {

lldb/source/Plugins/MemoryHistory/asan/MemoryHistoryASan.cpp

 }
 
 const char *memory_history_asan_command_prefix = R"(
     extern "C"
     {
         size_t __asan_get_alloc_stack(void *addr, void **trace, size_t size, int *thread_id);
         size_t __asan_get_free_stack(void *addr, void **trace, size_t size, int *thread_id);
     }
+)";
 
-    struct data {
+const char *memory_history_asan_command_format =
+    R"(
+    struct {
         void *alloc_trace[256];
         size_t alloc_count;
         int alloc_tid;
 
         void *free_trace[256];
         size_t free_count;
         int free_tid;
-    };
-)";
-
-const char *memory_history_asan_command_format =
-    R"(
-    data t;
+    } t;
 
     t.alloc_count = __asan_get_alloc_stack((void *)0x%)" PRIx64
     R"(, t.alloc_trace, 256, &t.alloc_tid);
     t.free_count = __asan_get_free_stack((void *)0x%)" PRIx64
     R"(, t.free_trace, 256, &t.free_tid);
 
     t;
 )";