sanitizer_common: make internal/external headers compatible

[dvyukov]

Summary

This is a follow up to 4f3f4d672254
("sanitizer_common: fix __sanitizer_get_module_and_offset_for_pc signature mismatch")
which fixes a similar problem for msan build.

I am getting the following error compiling a unit test for code that
uses sanitizer_common headers and googletest transitively includes
sanitizer interface headers:

In file included from third_party/gwp_sanitizers/singlestep_test.cpp:3:
In file included from sanitizer_common/sanitizer_common.h:19:
sanitizer_interface_internal.h:41:5: error: typedef redefinition with different types
('struct sanitizer_sandbox_arguments' vs 'struct sanitizer_sandbox_arguments')

} __sanitizer_sandbox_arguments;

common_interface_defs.h:39:3: note: previous definition is here
} __sanitizer_sandbox_arguments;

[dvyukov]

compiler-rt/lib/sanitizer_common/sanitizer_interface_internal.h

40

This is ugly. I am open to suggestions.

[melver]

compiler-rt/lib/sanitizer_common/sanitizer_interface_internal.h

40

I am actually confused where gtest includes this - my git grep foo didn't yield anything, do you know where gtest needs it? Maybe there is a "reduced" gtest header that doesn't include sanitizer headers?

[dvyukov]

removed the interface header from sanitizer_common.h

[dvyukov]

PTAL

compiler-rt/lib/sanitizer_common/sanitizer_interface_internal.h

40

That's our internal version that pulls in lots of stuff.

[melver]

Note, __sanitizer_sandbox_arguments seems unused, i.e. I don't see any of its members used anywhere?

Can we just delete that struct completely from everywhere and just change the "args" to a void* everywhere?

Is this used in a non-LLVM codebase for something?

compiler-rt/lib/sanitizer_common/tests/sanitizer_common_test.cpp

21

Is this change still necessary?

[dvyukov]

In D119546#3314420, @melver wrote:

Note, __sanitizer_sandbox_arguments seems unused, i.e. I don't see any of its members used anywhere?

Can we just delete that struct completely from everywhere and just change the "args" to a void* everywhere?

Is this used in a non-LLVM codebase for something?

It's an interface struct. We can't delete. User code may be referring to it.

compiler-rt/lib/sanitizer_common/tests/sanitizer_common_test.cpp

21

Yes, to prevent future breakages.
It's a test. It detected my current breakage and the breakage I fixed in the previous commit.

[melver]

In D119546#3314456, @dvyukov wrote:

It's an interface struct. We can't delete. User code may be referring to it.

Makes sense.

Though the internal version could be deleted, but probably not worth it, should it be needed in future which would break everything again. This change is probably the most robust solution.

[thakis]

Hi, this breaks building hwasan on my bot: http://45.33.8.238/linux/68278/step_4.txt

That bot uses GN so it's possible it's a bug on my end. But from what I can tell, there's no build file shenanigans here—__sanitizer_set_report_path is declared in compiler-rt/lib/sanitizer_common/sanitizer_interface_internal.h and compiler-rt/lib/hwasan/hwasan.cpp just doesn't include that. So maybe it's an oversight in this patch?

[paulkirth]

we're seeing a similar break in Fuchsia's clang canaries using CMake.

Bot can be found here: https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8822498877893075121/overview

[dvyukov]

Hi, I've mailed D119570 to fix this. Thanks for reporting.

[glandium]

compiler-rt/lib/sanitizer_common/sanitizer_file.cpp

22 ↗

(On Diff #407948)

this isn't aligned with the others.

[dvyukov]

compiler-rt/lib/sanitizer_common/sanitizer_file.cpp

22 ↗

(On Diff #407948)

This is a question to clang-format/arc lint. arc does not let me to mail/land unlinted changes and that's what it did:

>>> Lint for compiler-rt/lib/sanitizer_common/sanitizer_file.cpp:

 Auto-Fix  (S&RX) Lint
  clang-format suggested style edits found:

            19 
            20 #include "sanitizer_common.h"
            21 #include "sanitizer_file.h"
  >>> -     22 #include "sanitizer_interface_internal.h"
      +        #  include "sanitizer_interface_internal.h"
            23 
            24 namespace __sanitizer {
            25 
            26 void CatastrophicErrorWrite(const char *buffer, uptr length) {

compiler-rt/lib/sanitizer_common/sanitizer_interface_internal.h

extern "C" {
   // (casted to void *).
   SANITIZER_INTERFACE_ATTRIBUTE
   void __sanitizer_set_report_fd(void *fd);
   // Get the current full report file path, if a path was specified by
   // an earlier call to __sanitizer_set_report_path. Returns null otherwise.
   SANITIZER_INTERFACE_ATTRIBUTE
   const char *__sanitizer_get_report_path();
 
+  // common_interface_defs.h may be already included in unit tests,
+  // which include googletest headers, which may transitively include
+  // sanitizer interface headers. Avoid defining this struct second time
+  // in such case, or it will lead to typedef redefinition error.
+#ifndef SANITIZER_COMMON_INTERFACE_DEFS_H
   typedef struct {
       int coverage_sandboxed;
       __sanitizer::sptr coverage_fd;
       unsigned int coverage_max_block_size;
   } __sanitizer_sandbox_arguments;
+#endif
 
   // Notify the tools that the sandbox is going to be turned on.
   SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE void
       __sanitizer_sandbox_on_notify(__sanitizer_sandbox_arguments *args);
 
   // This function is called by the tool when it has just finished reporting
   // an error. 'error_summary' is a one-line string that summarizes
   // the error message. This function can be overridden by the client.

compiler-rt/lib/sanitizer_common/tests/sanitizer_common_test.cpp

 //===-- sanitizer_common_test.cpp -----------------------------------------===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 // This file is a part of ThreadSanitizer/AddressSanitizer runtime.
 //
 //===----------------------------------------------------------------------===//
 #include <algorithm>
 
+// This ensures that including both internal sanitizer_common headers
+// and the interface headers does not lead to compilation failures.
+// Both may be included in unit tests, where googletest transitively
+// pulls in sanitizer interface headers.
+// The headers are specifically included using relative paths,
+// because a compiler may use a different mismatching version
+// of sanitizer headers.
+#include "../../../include/sanitizer/asan_interface.h"
+#include "../../../include/sanitizer/msan_interface.h"
+#include "../../../include/sanitizer/tsan_interface.h"
+#include "gtest/gtest.h"
 #include "sanitizer_common/sanitizer_allocator_internal.h"
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_file.h"
 #include "sanitizer_common/sanitizer_flags.h"
 #include "sanitizer_common/sanitizer_libc.h"
 #include "sanitizer_common/sanitizer_platform.h"
-
 #include "sanitizer_pthread_wrappers.h"
 
-#include "gtest/gtest.h"
-
 namespace __sanitizer {
 
 static bool IsSorted(const uptr *array, uptr n) {
   for (uptr i = 1; i < n; i++) {
     if (array[i] < array[i - 1]) return false;
   }
   return true;
 }