[compiler-rt][hwasan] Remove __sanitizer allocation functions from hwasan interface

[leonardchan]

Summary

These functions should not be externally used. We also do not need them internally for Fuchsia.

[mcgrathr]

I don't think this is warranted. There should not be any references to such things when building for Fuchsia.
This looks like it's intended to satisfy one reference in dead code in hwasan_interceptors.cpp.
I think the right solution is just to refactor the hwasan code so that the dead code is not compiled for Fuchsia at all.

[leonardchan]

In D99381#2651896, @mcgrathr wrote:

I don't think this is warranted. There should not be any references to such things when building for Fuchsia.
This looks like it's intended to satisfy one reference in dead code in hwasan_interceptors.cpp.
I think the right solution is just to refactor the hwasan code so that the dead code is not compiled for Fuchsia at all.

To add more context, this patch was meant to resolve this missing type error:

[27/32] Building CXX object compiler-rt/lib/hwasan/CMakeFiles/RTHwasan_dynamic.aarch64.dir/hwasan.cpp.obj
FAILED: compiler-rt/lib/hwasan/CMakeFiles/RTHwasan_dynamic.aarch64.dir/hwasan.cpp.obj 
/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-4-master-fuchsia-toolchain/./bin/clang++ --target=aarch64-unknown-fuchsia --sysroot=/usr/local/google/home/leonardchan/sdk/garnet/arch/arm64/sysroot -DHWASAN_REPLACE_OPERATORS_NEW_AND_DELETE=1 -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-4/compiler-rt/lib/hwasan/.. --target=aarch64-unknown-fuchsia -I/usr/local/google/home/leonardchan/sdk/garnet/pkg/fdio/include -fPIC -fvisibility-inlines-hidden -Werror=date-time -Werror=unguarded-availability-new -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wmissing-field-initializers -Wimplicit-fallthrough -Wcovered-switch-default -Wno-noexcept-type -Wnon-virtual-dtor -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wstring-conversion -ffunction-sections -fdata-sections -ffile-prefix-map=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-build-4-master-fuchsia-toolchain/runtimes/runtimes-aarch64-unknown-fuchsia-bins=../llvm-build-4-master-fuchsia-toolchain/runtimes/runtimes-aarch64-unknown-fuchsia-bins -ffile-prefix-map=/usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-4/= -no-canonical-prefixes -Wall -std=c++14 -Wno-unused-parameter -O2 -g -DNDEBUG  -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fno-sanitize=safe-stack -fvisibility=hidden -fno-lto -O3 -gline-tables-only -Wno-gnu -Wno-variadic-macros -Wno-c99-extensions -nostdinc++ -fno-rtti -fPIC -ffreestanding -ftls-model=initial-exec -MD -MT compiler-rt/lib/hwasan/CMakeFiles/RTHwasan_dynamic.aarch64.dir/hwasan.cpp.obj -MF compiler-rt/lib/hwasan/CMakeFiles/RTHwasan_dynamic.aarch64.dir/hwasan.cpp.obj.d -o compiler-rt/lib/hwasan/CMakeFiles/RTHwasan_dynamic.aarch64.dir/hwasan.cpp.obj -c /usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-4/compiler-rt/lib/hwasan/hwasan.cpp
In file included from /usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-4/compiler-rt/lib/hwasan/hwasan.cpp:14:
In file included from /usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-4/compiler-rt/lib/hwasan/hwasan.h:20:
/usr/local/google/home/leonardchan/llvm-monorepo/llvm-project-4/compiler-rt/lib/hwasan/hwasan_interface_internal.h:204:11: error: no type named '__sanitizer_struct_mallinfo' in namespace '__hwasan'
__hwasan::__sanitizer_struct_mallinfo __sanitizer_mallinfo();
~~~~~~~~~~^

__sanitizer_struct_mallinfo is used as part of the hwasan interface:

SANITIZER_INTERFACE_ATTRIBUTE
__hwasan::__sanitizer_struct_mallinfo __sanitizer_mallinfo();

So I'm thinking unless we want to change the API, we may not want to change or remove this function. The error comes up because __sanitizer_struct_mallinfo is currently only defined if SANITIZER_LINUX || SANITIZER_MAC is true, so it felt appropriate to add a fuchsia-specific definition in sanitizer_platform_limits_fuchsia.h which can just be empty if we don't use it anyway. Would refactoring still be applicable in this case?

[phosek]

I see, it's exposed here https://github.com/llvm/llvm-project/blob/6d2d3bd0a61f5fc7fd9f61f48bc30e9ca77cc619/compiler-rt/include/sanitizer/hwasan_interface.h#L88.

[leonardchan]

*ping* @mcgrathr Given my explanation before, does it still make sense to refactor in this case?

[vitalybuka]

compiler-rt/lib/hwasan/hwasan_interceptors.cpp

28

we usually include them unconditionaly and .h itself has guards against unsupported platforms

[leonardchan]

we usually include them unconditionaly and .h itself has guards against unsupported platforms

@vitalybuka Did you mean something like this? Definitely simplifies the patch.

[phosek]

What I think @vitalybuka meant is keeping sanitizer_platform_limits_fuchsia.h as you had it, but including it unconditionally. Since the entire file is wrapped in #if SANITIZER_FUCHSIA ... #endif, the inclusion would be a no-op on platforms other than Fuchsia so no need to wrap the #include in #if SANITIZER_FUCHSIA as well.

[vitalybuka]

In D99381#2721619, @phosek wrote:

What I think @vitalybuka meant is keeping sanitizer_platform_limits_fuchsia.h as you had it, but including it unconditionally. Since the entire file is wrapped in #if SANITIZER_FUCHSIA ... #endif, the inclusion would be a no-op on platforms other than Fuchsia so no need to wrap the #include in #if SANITIZER_FUCHSIA as well.

Correct., Just:

#include "sanitizer_common/sanitizer_platform_limits_fuchsia.h"
#include "sanitizer_common/sanitizer_platform_limits_posix.h"

[leonardchan]

In D99381#2723563, @vitalybuka wrote:

In D99381#2721619, @phosek wrote:

What I think @vitalybuka meant is keeping sanitizer_platform_limits_fuchsia.h as you had it, but including it unconditionally. Since the entire file is wrapped in #if SANITIZER_FUCHSIA ... #endif, the inclusion would be a no-op on platforms other than Fuchsia so no need to wrap the #include in #if SANITIZER_FUCHSIA as well.

Correct., Just:

#include "sanitizer_common/sanitizer_platform_limits_fuchsia.h"
#include "sanitizer_common/sanitizer_platform_limits_posix.h"

I see. Thanks for the clarifications.

[leonardchan]

*ping*

[mcgrathr]

Frankly I don't think it makes sense to have __sanitizer_mallinfo and those other allocator functions declared in hwasan_interface_internal.h at all.
Those are neither APIs that the compiler emits nor ones that anyone else should use. They are just implementation details of the allocator interceptors.
I think a better cleanup is to move those declarations out of that file. I don't see why they need to be in any header file rather than just in hwasan_allocation_functions.cpp.

[leonardchan]

In D99381#2858599, @mcgrathr wrote:

Frankly I don't think it makes sense to have __sanitizer_mallinfo and those other allocator functions declared in hwasan_interface_internal.h at all.
Those are neither APIs that the compiler emits nor ones that anyone else should use. They are just implementation details of the allocator interceptors.
I think a better cleanup is to move those declarations out of that file. I don't see why they need to be in any header file rather than just in hwasan_allocation_functions.cpp.

I see. So something like this?

[vitalybuka]

@eugenis Why do we have them in here and in sanitizer/hwasan_interface.h ?

[vitalybuka]

In D99381#2908102, @vitalybuka wrote:

@eugenis Why do we have them in here and in sanitizer/hwasan_interface.h ?

And if we don't need them in sanitizer/hwasan_interface.h and interface_internal,
why they need SANITIZER_INTERFACE_ATTRIBUTE

I can't find any callers other than compiler-rt/test/hwasan/TestCases/sanitizer_malloc.cpp
Could we remove these functions (including implementation)?

[eugenis]

They are used here:
https://cs.android.com/android/platform/superproject/+/master:bionic/libc/bionic/malloc_common.h;l=54;drc=f3968e89cb72400951f93a2a8237ac1428d2627c

[vitalybuka]

In D99381#2911057, @eugenis wrote:

They are used here:
https://cs.android.com/android/platform/superproject/+/master:bionic/libc/bionic/malloc_common.h;l=54;drc=f3968e89cb72400951f93a2a8237ac1428d2627c

Thanks, it's preprocessor generated, so I can't find it :)

If so I guess LGTM.
But now they are in the same file, so instead of declarations, maybe just place SANITIZER_INTERFACE_ATTRIBUTE next to to the definition?

[vitalybuka]

compiler-rt/lib/hwasan/hwasan_allocation_functions.cpp

20 ↗

(On Diff #362103)

isn't HWASAN_WITH_INTERCEPTORS enough?

[vitalybuka]

compiler-rt/lib/hwasan/hwasan_allocation_functions.cpp

20 ↗

(On Diff #362103)

Ignore this. !SANITIZER_FUCHSIA is OK here.

[leonardchan]

In D99381#2911459, @vitalybuka wrote:

In D99381#2911057, @eugenis wrote:

They are used here:
https://cs.android.com/android/platform/superproject/+/master:bionic/libc/bionic/malloc_common.h;l=54;drc=f3968e89cb72400951f93a2a8237ac1428d2627c

Thanks, it's preprocessor generated, so I can't find it :)

If so I guess LGTM.
But now they are in the same file, so instead of declarations, maybe just place SANITIZER_INTERFACE_ATTRIBUTE next to to the definition?

Done. I'll take this as a review LGTM and submit tomorrow unless others have comments.

compiler-rt/lib/hwasan/hwasan_interceptors.cpp

 
 #include "interception/interception.h"
 #include "hwasan.h"
 #include "hwasan_allocator.h"
 #include "hwasan_mapping.h"
 #include "hwasan_thread.h"
 #include "hwasan_poisoning.h"
 #include "hwasan_report.h"
+#if SANITIZER_FUCHSIA
+#include "sanitizer_common/sanitizer_platform_limits_fuchsia.h"
+#endif
+#if SANITIZER_POSIX
 #include "sanitizer_common/sanitizer_platform_limits_posix.h"
+#endif
 #include "sanitizer_common/sanitizer_allocator.h"
 #include "sanitizer_common/sanitizer_allocator_interface.h"
 #include "sanitizer_common/sanitizer_allocator_internal.h"
 #include "sanitizer_common/sanitizer_atomic.h"
 #include "sanitizer_common/sanitizer_common.h"
 #include "sanitizer_common/sanitizer_errno.h"
 #include "sanitizer_common/sanitizer_stackdepot.h"
 #include "sanitizer_common/sanitizer_libc.h"

compiler-rt/lib/hwasan/hwasan_interface_internal.h

 //
 // Private Hwasan interface header.
 //===----------------------------------------------------------------------===//
 
 #ifndef HWASAN_INTERFACE_INTERNAL_H
 #define HWASAN_INTERFACE_INTERNAL_H
 
 #include "sanitizer_common/sanitizer_internal_defs.h"
+#if SANITIZER_FUCHSIA
+#include "sanitizer_common/sanitizer_platform_limits_fuchsia.h"
+#endif
+#if SANITIZER_POSIX
 #include "sanitizer_common/sanitizer_platform_limits_posix.h"
+#endif
 #include <link.h>
 
 extern "C" {
 
 SANITIZER_INTERFACE_ATTRIBUTE
 void __hwasan_init_static();
 
 SANITIZER_INTERFACE_ATTRIBUTE

compiler-rt/lib/sanitizer_common/sanitizer_platform_limits_fuchsia.h

+//===-- sanitizer_platform_limits_fuchsia.h -------------------------------===//
+//
+// 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 Sanitizer common code.
+//
+// Sizes and layouts of platform-specific Fuchsia data structures.
+//===----------------------------------------------------------------------===//
+
+#ifndef SANITIZER_PLATFORM_LIMITS_FUCHSIA_H
+#define SANITIZER_PLATFORM_LIMITS_FUCHSIA_H
+
+#if SANITIZER_FUCHSIA
+
+namespace __sanitizer {
+struct __sanitizer_struct_mallinfo {};
+}  // namespace __sanitizer
+
+#endif  // SANITIZER_FUCHSIA
+
+#endif