This is an archive of the discontinued LLVM Phabricator instance.

sanitizer_common: make internal/external headers compatible
ClosedPublic

Authored by dvyukov on Feb 11 2022, 7:16 AM.

Details

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;

Diff Detail

Event Timeline

dvyukov requested review of this revision.Feb 11 2022, 7:16 AM
dvyukov created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 11 2022, 7:16 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
dvyukov added inline comments.Feb 11 2022, 7:17 AM
compiler-rt/lib/sanitizer_common/sanitizer_interface_internal.h
40 ↗(On Diff #407873)

This is ugly. I am open to suggestions.

melver added inline comments.Feb 11 2022, 7:51 AM
compiler-rt/lib/sanitizer_common/sanitizer_interface_internal.h
40 ↗(On Diff #407873)

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 updated this revision to Diff 407887.Feb 11 2022, 8:20 AM

removed the interface header from sanitizer_common.h

dvyukov marked an inline comment as done.Feb 11 2022, 8:21 AM

PTAL

compiler-rt/lib/sanitizer_common/sanitizer_interface_internal.h
40 ↗(On Diff #407873)

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

melver accepted this revision.Feb 11 2022, 8:53 AM

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?

This revision is now accepted and ready to land.Feb 11 2022, 8:53 AM
dvyukov marked an inline comment as done.Feb 11 2022, 9:00 AM

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.

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.

This revision was landed with ongoing or failed builds.Feb 11 2022, 10:39 AM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Feb 11 2022, 11:00 AM

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?

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

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

glandium added inline comments.
compiler-rt/lib/sanitizer_common/sanitizer_file.cpp
22

this isn't aligned with the others.

dvyukov added inline comments.Feb 12 2022, 1:19 AM
compiler-rt/lib/sanitizer_common/sanitizer_file.cpp
22

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) {