From e4d4153352ba60896f3d8e585ce79f0681f66a23 Mon Sep 17 00:00:00 2001 From: Yuval Tassa Date: Tue, 10 Sep 2024 09:04:19 -0700 Subject: [PATCH] Move sanitizer instrumentation to a separate header file. Fixes #2049. PiperOrigin-RevId: 672986547 Change-Id: I42522f4925237a73168e965364a2dd65c0f067cb --- CMakeLists.txt | 1 + doc/programming/index.rst | 10 +++-- include/mujoco/mjmacro.h | 11 ------ include/mujoco/mjsan.h | 58 ++++++++++++++++++++++++++++ include/mujoco/mujoco.h | 32 +-------------- src/engine/engine_collision_driver.c | 1 + src/engine/engine_collision_sdf.c | 1 + src/engine/engine_core_constraint.c | 1 + src/engine/engine_core_smooth.c | 1 + src/engine/engine_derivative.c | 1 + src/engine/engine_derivative_fd.c | 1 + src/engine/engine_forward.c | 1 + src/engine/engine_inverse.c | 1 + src/engine/engine_io.c | 1 + src/engine/engine_island.c | 1 + src/engine/engine_print.c | 1 + src/engine/engine_ray.c | 1 + src/engine/engine_sensor.c | 1 + src/engine/engine_setconst.c | 1 + src/engine/engine_solver.c | 1 + src/engine/engine_support.c | 1 + src/engine/engine_util_solve.c | 1 + src/engine/engine_util_sparse.c | 1 + src/engine/engine_vis_interact.c | 1 + src/engine/engine_vis_visualize.c | 1 + src/thread/thread_pool.cc | 1 + unity/Runtime/Bindings/MjBindings.cs | 1 + 27 files changed, 89 insertions(+), 45 deletions(-) create mode 100644 include/mujoco/mjsan.h diff --git a/CMakeLists.txt b/CMakeLists.txt index 54be24c7..a7b3816f 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -59,6 +59,7 @@ set(MUJOCO_HEADERS include/mujoco/mjmodel.h include/mujoco/mjplugin.h include/mujoco/mjrender.h + include/mujoco/mjsan.h include/mujoco/mjspec.h include/mujoco/mjthread.h include/mujoco/mjtnum.h diff --git a/doc/programming/index.rst b/doc/programming/index.rst index 03916e22..f1658321 100644 --- a/doc/programming/index.rst +++ b/doc/programming/index.rst @@ -148,6 +148,10 @@ links below, to make this documentation self-contained. Defines MuJoCo's ``mjtNum`` floating-point type to be either ``double`` or ``float``. See :ref:`mjtNum`. `mjspec.h `__ Defines enums and structs used for :doc:`procedural model editing `. +`mjplugin.h `__ + Defines data structures required by :ref:`engine plugins`. +`mjthread.h `__ + Defines data structures and functions required by :ref:`thread`. `mjmacro.h `__ Defines C macros that are useful in user code. `mjxmacro.h `__ @@ -157,10 +161,8 @@ links below, to make this documentation self-contained. `mjexport.h `__ Macros used for exporting public symbols from the MuJoCo library. This header should not be used directly by client code. -`mjplugin.h `__ - Defines data structures required by :ref:`engine plugins`. -`mjthread.h `__ - Defines data structures and functions required by :ref:`thread`. +`mjsan.h `__ + Definitions required when building with sanitizer instrumentation. .. _inVersion: diff --git a/include/mujoco/mjmacro.h b/include/mujoco/mjmacro.h index d6988292..a50c117c 100644 --- a/include/mujoco/mjmacro.h +++ b/include/mujoco/mjmacro.h @@ -15,17 +15,6 @@ #ifndef MUJOCO_MJMACRO_H_ #define MUJOCO_MJMACRO_H_ -// include asan interface header, or provide stubs for poison/unpoison macros when not using asan -#ifdef ADDRESS_SANITIZER - #include -#elif defined(_MSC_VER) - #define ASAN_POISON_MEMORY_REGION(addr, size) - #define ASAN_UNPOISON_MEMORY_REGION(addr, size) -#else - #define ASAN_POISON_MEMORY_REGION(addr, size) ((void)(addr), (void)(size)) - #define ASAN_UNPOISON_MEMORY_REGION(addr, size) ((void)(addr), (void)(size)) -#endif - // max and min (use only for primitive types) #define mjMAX(a, b) (((a) > (b)) ? (a) : (b)) #define mjMIN(a, b) (((a) < (b)) ? (a) : (b)) diff --git a/include/mujoco/mjsan.h b/include/mujoco/mjsan.h new file mode 100644 index 00000000..4d45997b --- /dev/null +++ b/include/mujoco/mjsan.h @@ -0,0 +1,58 @@ +// Copyright 2024 DeepMind Technologies Limited +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef MUJOCO_INCLUDE_MJSAN_H_ +#define MUJOCO_INCLUDE_MJSAN_H_ + +// Include asan interface header, or provide stubs for poison/unpoison macros when not using asan. +#ifdef ADDRESS_SANITIZER + #include +#elif defined(_MSC_VER) + #define ASAN_POISON_MEMORY_REGION(addr, size) + #define ASAN_UNPOISON_MEMORY_REGION(addr, size) +#else + #define ASAN_POISON_MEMORY_REGION(addr, size) ((void)(addr), (void)(size)) + #define ASAN_UNPOISON_MEMORY_REGION(addr, size) ((void)(addr), (void)(size)) +#endif + +// When built and run under address sanitizer (asan), mj_markStack and mj_freeStack are instrumented +// to detect leakage of mjData stack frames. When the compiler inlines several callees that call +// into mark/free into the same function, this instrumentation requires that the compiler retains +// separate mark/free calls for each original callee. The memory-clobbered asm blocks act as a +// barrier to prevent mark/free calls from being combined under optimization. +#ifdef ADDRESS_SANITIZER +#ifdef __cplusplus +extern "C" { +#endif + +void mj__markStack(mjData*) __attribute__((noinline)); +static inline void mj_markStack(mjData* d) __attribute__((always_inline)) { + asm volatile("" ::: "memory"); + mj__markStack(d); + asm volatile("" ::: "memory"); +} + +void mj__freeStack(mjData*) __attribute__((noinline)); +static inline void mj_freeStack(mjData* d) __attribute__((always_inline)) { + asm volatile("" ::: "memory"); + mj__freeStack(d); + asm volatile("" ::: "memory"); +} + +#ifdef __cplusplus +} +#endif // __cplusplus +#endif // ADDRESS_SANITIZER + +#endif // MUJOCO_INCLUDE_MJSAN_H_ diff --git a/include/mujoco/mujoco.h b/include/mujoco/mujoco.h index 1e6b5431..fbf9b151 100644 --- a/include/mujoco/mujoco.h +++ b/include/mujoco/mujoco.h @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -197,7 +198,7 @@ MJAPI void mj_resetDataDebug(const mjModel* m, mjData* d, unsigned char debug_va // Reset data. If 0 <= key < nkey, set fields from specified keyframe. MJAPI void mj_resetDataKeyframe(const mjModel* m, mjData* d, int key); -#ifndef ADDRESS_SANITIZER +#ifndef ADDRESS_SANITIZER // Stack management functions declared in mjsan.h if ASAN is active. // Mark a new frame on the mjData stack. MJAPI void mj_markStack(mjData* d); @@ -1766,35 +1767,6 @@ MJAPI void mjs_defaultKey(mjsKey* key); // Default plugin attributes. MJAPI void mjs_defaultPlugin(mjsPlugin* plugin); - -//---------------------------------- Sanitizer instrumentation ------------------------------------- - -// Most users can ignore these functions, the following comments are primarily for developers. -// -// When built and run under address sanitizer (asan), mj_markStack and mj_freeStack are instrumented -// to detect leakage of mjData stack frames. When the compiler inlines several callees that call -// into mark/free into the same function, this instrumentation requires that the compiler retains -// separate mark/free calls for each original callee. The memory-clobbered asm blocks act as a -// barrier to prevent mark/free calls from being combined under optimization. - -#ifdef ADDRESS_SANITIZER - -void mj__markStack(mjData*) __attribute__((noinline)); -static inline void mj_markStack(mjData* d) __attribute__((always_inline)) { - asm volatile("" ::: "memory"); - mj__markStack(d); - asm volatile("" ::: "memory"); -} - -void mj__freeStack(mjData*) __attribute__((noinline)); -static inline void mj_freeStack(mjData* d) __attribute__((always_inline)) { - asm volatile("" ::: "memory"); - mj__freeStack(d); - asm volatile("" ::: "memory"); -} - -#endif // ADDRESS_SANITIZER - #ifdef __cplusplus } #endif diff --git a/src/engine/engine_collision_driver.c b/src/engine/engine_collision_driver.c index 6f4cab53..3f5e548b 100644 --- a/src/engine/engine_collision_driver.c +++ b/src/engine/engine_collision_driver.c @@ -21,6 +21,7 @@ #include #include #include +#include // IWYU pragma: keep #include "engine/engine_callback.h" #include "engine/engine_collision_convex.h" #include "engine/engine_collision_primitive.h" diff --git a/src/engine/engine_collision_sdf.c b/src/engine/engine_collision_sdf.c index 4bb2de3b..8396b61e 100644 --- a/src/engine/engine_collision_sdf.c +++ b/src/engine/engine_collision_sdf.c @@ -19,6 +19,7 @@ #include #include +#include // IWYU pragma: keep #include #include "engine/engine_collision_primitive.h" #include "engine/engine_io.h" diff --git a/src/engine/engine_core_constraint.c b/src/engine/engine_core_constraint.c index 74f60634..ab583a6e 100644 --- a/src/engine/engine_core_constraint.c +++ b/src/engine/engine_core_constraint.c @@ -20,6 +20,7 @@ #include #include #include +#include // IWYU pragma: keep #include #include "engine/engine_core_smooth.h" #include "engine/engine_io.h" diff --git a/src/engine/engine_core_smooth.c b/src/engine/engine_core_smooth.c index 5602eece..0b8d097d 100644 --- a/src/engine/engine_core_smooth.c +++ b/src/engine/engine_core_smooth.c @@ -20,6 +20,7 @@ #include #include #include +#include // IWYU pragma: keep #include "engine/engine_core_constraint.h" #include "engine/engine_crossplatform.h" #include "engine/engine_io.h" diff --git a/src/engine/engine_derivative.c b/src/engine/engine_derivative.c index b321ba88..c49e9484 100644 --- a/src/engine/engine_derivative.c +++ b/src/engine/engine_derivative.c @@ -16,6 +16,7 @@ #include #include +#include // IWYU pragma: keep #include "engine/engine_core_constraint.h" #include "engine/engine_crossplatform.h" #include "engine/engine_io.h" diff --git a/src/engine/engine_derivative_fd.c b/src/engine/engine_derivative_fd.c index 84126ffa..239779af 100644 --- a/src/engine/engine_derivative_fd.c +++ b/src/engine/engine_derivative_fd.c @@ -20,6 +20,7 @@ #include #include #include +#include // IWYU pragma: keep #include "engine/engine_forward.h" #include "engine/engine_io.h" #include "engine/engine_inverse.h" diff --git a/src/engine/engine_forward.c b/src/engine/engine_forward.c index 16d5d182..ae4b7c2d 100644 --- a/src/engine/engine_forward.c +++ b/src/engine/engine_forward.c @@ -20,6 +20,7 @@ #include #include #include +#include // IWYU pragma: keep #include #include "engine/engine_callback.h" #include "engine/engine_collision_driver.h" diff --git a/src/engine/engine_inverse.c b/src/engine/engine_inverse.c index 8805f8e4..f582728e 100644 --- a/src/engine/engine_inverse.c +++ b/src/engine/engine_inverse.c @@ -19,6 +19,7 @@ #include #include #include +#include // IWYU pragma: keep #include "engine/engine_collision_driver.h" #include "engine/engine_core_constraint.h" #include "engine/engine_core_smooth.h" diff --git a/src/engine/engine_io.c b/src/engine/engine_io.c index 68a056f1..b737300e 100644 --- a/src/engine/engine_io.c +++ b/src/engine/engine_io.c @@ -25,6 +25,7 @@ #include #include #include +#include // IWYU pragma: keep #include #include "engine/engine_crossplatform.h" #include "engine/engine_macro.h" diff --git a/src/engine/engine_island.c b/src/engine/engine_island.c index 7a11735f..286af4c7 100644 --- a/src/engine/engine_island.c +++ b/src/engine/engine_island.c @@ -19,6 +19,7 @@ #include #include +#include // IWYU pragma: keep #include #include "engine/engine_core_constraint.h" #include "engine/engine_io.h" diff --git a/src/engine/engine_print.c b/src/engine/engine_print.c index a12ccdb4..b9edf0e4 100644 --- a/src/engine/engine_print.c +++ b/src/engine/engine_print.c @@ -22,6 +22,7 @@ #include #include #include +#include // IWYU pragma: keep #include #include "engine/engine_core_constraint.h" #include "engine/engine_io.h" diff --git a/src/engine/engine_ray.c b/src/engine/engine_ray.c index 8c341825..13e26389 100644 --- a/src/engine/engine_ray.c +++ b/src/engine/engine_ray.c @@ -21,6 +21,7 @@ #include #include #include +#include // IWYU pragma: keep #include #include "engine/engine_io.h" #include "engine/engine_plugin.h" diff --git a/src/engine/engine_sensor.c b/src/engine/engine_sensor.c index 415472dc..43e645b3 100644 --- a/src/engine/engine_sensor.c +++ b/src/engine/engine_sensor.c @@ -19,6 +19,7 @@ #include #include #include +#include // IWYU pragma: keep #include "engine/engine_callback.h" #include "engine/engine_core_smooth.h" #include "engine/engine_crossplatform.h" diff --git a/src/engine/engine_setconst.c b/src/engine/engine_setconst.c index 8bb19739..5815fea8 100644 --- a/src/engine/engine_setconst.c +++ b/src/engine/engine_setconst.c @@ -20,6 +20,7 @@ #include #include #include +#include // IWYU pragma: keep #include "engine/engine_core_constraint.h" #include "engine/engine_core_smooth.h" #include "engine/engine_forward.h" diff --git a/src/engine/engine_solver.c b/src/engine/engine_solver.c index 99997b74..a5c652a8 100644 --- a/src/engine/engine_solver.c +++ b/src/engine/engine_solver.c @@ -20,6 +20,7 @@ #include #include #include +#include // IWYU pragma: keep #include "engine/engine_core_constraint.h" #include "engine/engine_core_smooth.h" #include "engine/engine_io.h" diff --git a/src/engine/engine_support.c b/src/engine/engine_support.c index 13dca21d..ad2a8762 100644 --- a/src/engine/engine_support.c +++ b/src/engine/engine_support.c @@ -19,6 +19,7 @@ #include #include +#include // IWYU pragma: keep #include "engine/engine_collision_driver.h" #include "engine/engine_core_constraint.h" #include "engine/engine_crossplatform.h" diff --git a/src/engine/engine_util_solve.c b/src/engine/engine_util_solve.c index f6da3133..03c0bebd 100644 --- a/src/engine/engine_util_solve.c +++ b/src/engine/engine_util_solve.c @@ -19,6 +19,7 @@ #include #include +#include // IWYU pragma: keep #include "engine/engine_io.h" #include "engine/engine_util_blas.h" #include "engine/engine_util_errmem.h" diff --git a/src/engine/engine_util_sparse.c b/src/engine/engine_util_sparse.c index 6a804ac2..c12132d0 100644 --- a/src/engine/engine_util_sparse.c +++ b/src/engine/engine_util_sparse.c @@ -18,6 +18,7 @@ #include #include +#include // IWYU pragma: keep #include #include "engine/engine_io.h" #include "engine/engine_util_blas.h" diff --git a/src/engine/engine_vis_interact.c b/src/engine/engine_vis_interact.c index b485d46a..4803c525 100644 --- a/src/engine/engine_vis_interact.c +++ b/src/engine/engine_vis_interact.c @@ -20,6 +20,7 @@ #include #include #include +#include // IWYU pragma: keep #include #include "engine/engine_core_smooth.h" #include "engine/engine_io.h" diff --git a/src/engine/engine_vis_visualize.c b/src/engine/engine_vis_visualize.c index 8155225e..a09e76cb 100644 --- a/src/engine/engine_vis_visualize.c +++ b/src/engine/engine_vis_visualize.c @@ -20,6 +20,7 @@ #include #include #include +#include // IWYU pragma: keep #include #include "engine/engine_array_safety.h" #include "engine/engine_io.h" diff --git a/src/thread/thread_pool.cc b/src/thread/thread_pool.cc index 9bb03133..af117f6a 100644 --- a/src/thread/thread_pool.cc +++ b/src/thread/thread_pool.cc @@ -25,6 +25,7 @@ #include #include +#include // IWYU pragma: keep #include #include #include "engine/engine_crossplatform.h" diff --git a/unity/Runtime/Bindings/MjBindings.cs b/unity/Runtime/Bindings/MjBindings.cs index 7598b0e6..70eb8412 100644 --- a/unity/Runtime/Bindings/MjBindings.cs +++ b/unity/Runtime/Bindings/MjBindings.cs @@ -56,6 +56,7 @@ public const bool THIRD_PARTY_MUJOCO_MJRENDER_H_ = true; public const int mjNAUX = 10; public const int mjMAXTEXTURE = 1000; public const int mjMAXMATERIAL = 1000; +public const bool THIRD_PARTY_MUJOCO_INCLUDE_MJSAN_H_ = true; public const bool THIRD_PARTY_MUJOCO_INCLUDE_MJSPEC_H_ = true; public const bool THIRD_PARTY_MUJOCO_INCLUDE_MJTHREAD_H_ = true; public const int mjMAXTHREAD = 128;