This is an archive of the discontinued LLVM Phabricator instance.

[sanitizer] Move MSan's chained_origin_depot to sanitizer_common
ClosedPublic

Authored by stephan.yichao.zhao on Feb 8 2021, 9:53 PM.

Details

Summary

https://reviews.llvm.org/D95835 implements origin tracking for DFSan.
It reuses the chained origin depot of MSan.

This change moves the utility to sanitizer_common to share between
MSan and DFSan.

Diff Detail

Event Timeline

stephan.yichao.zhao requested review of this revision.Feb 8 2021, 9:53 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2021, 9:53 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
stephan.yichao.zhao retitled this revision from [sanitizer] Remove MSan's chained_origin_depot to sanitizer_common to [sanitizer] Move MSan's chained_origin_depot to sanitizer_common.Feb 8 2021, 9:58 PM
morehouse added inline comments.Feb 9 2021, 11:01 AM
compiler-rt/lib/sanitizer_common/sanitizer_chained_origin_depot.cpp
2

Looks like accidental line break here.

compiler-rt/lib/sanitizer_common/tests/sanitizer_chained_origin_depot_test.cpp
17

Please fix this lint

45

Do we need to somehow reset the origin depot between tests? If the Basic test runs before this one, will we still get true?

48

So we expect that when the Put fails it should return the existing origin ID. Let's document this in the ChainedOriginDepotPut function comment.

60

Should we test that Get with both new_id1 and new_id2 returns the current StackDepot ID and previous origin ID?

stephan.yichao.zhao marked 5 inline comments as done.Feb 9 2021, 12:29 PM
stephan.yichao.zhao added inline comments.
compiler-rt/lib/sanitizer_common/tests/sanitizer_chained_origin_depot_test.cpp
17

reordered them. but that gtest.h cannot be removed.

45

The underlying StackDepot is a global variable, and internally it allocates memory permanently, does not have a 'free' method.

This is a good point. I updated the test cases to use different numbers.

stephan.yichao.zhao marked 2 inline comments as done.
stephan.yichao.zhao retitled this revision from [sanitizer] Move MSan's chained_origin_depot to sanitizer_common to [sanitizer] Move MSan's chained_origin_depot to sanitizer_common.

updated

compiler-rt/lib/sanitizer_common/tests/sanitizer_chained_origin_depot_test.cpp
17

for some reason, the local lint used by arc suggests this style different from the online one, and does not allow update if we reordered the headers.

This revision is now accepted and ready to land.Feb 9 2021, 12:35 PM
eugenis added inline comments.Feb 9 2021, 12:46 PM
compiler-rt/lib/sanitizer_common/sanitizer_chained_origin_depot.cpp
102

This is a pretty large global variable that seems to be added to all sanitizers now, even those that do not need it, like hwasan.

compiler-rt/lib/sanitizer_common/sanitizer_chained_origin_depot.cpp
102

This is a good point...

The main goal of this change is about code reuse.

To address this issue, one possible fixes could be making the declaration here as extern attribute((weak));, and msan/dsan can define them separately.
Other sanitizers would not define it. Will this work for msan?

I'd rather create a ChainedOriginDepot class, turn all the global functions into methods, turn the global variable into a member and then instantiate it in dfsan and msan separately.

stephan.yichao.zhao updated this revision to Diff 322540.EditedFeb 9 2021, 4:43 PM

changed origin chain to a class.

But we still keep the wrappers in msan_chained_origin_depot.h|cpp, because sanitizer_chained_origin_depot includes sanitizer_stackdepotbase and sanitizer_stackdepotbase includes sanitizer_internal_defs. sanitizer_internal_defs has types and functions conflicting with msan_interceptors

stephan.yichao.zhao marked an inline comment as done.Feb 9 2021, 4:46 PM
stephan.yichao.zhao requested review of this revision.Feb 10 2021, 2:55 PM
eugenis accepted this revision.Feb 10 2021, 5:10 PM

LGTM

compiler-rt/lib/sanitizer_common/sanitizer_chained_origin_depot.h
80

Please rename it to just "depot" or something like that - the current name makes it look like it could be an instance of class ChainOriginDepot.

This revision is now accepted and ready to land.Feb 10 2021, 5:10 PM
stephan.yichao.zhao marked an inline comment as done.Feb 10 2021, 5:22 PM

renamed chainedOriginDepot to depot.

This revision was landed with ongoing or failed builds.Feb 10 2021, 5:26 PM
This revision was automatically updated to reflect the committed changes.

This one breaks "check-memprof"

'RUN: at line 9';   env MEMPROF_OPTIONS=log_path=stderr  /usr/local/google/home/vitalybuka/src/llvm.git/out/z/projects/compiler-rt/test/memprof/X86_64LinuxConfig/TestCases/Output/test_malloc_load_store.c.tmp 2>&1 | FileCheck /usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/compiler-rt/test/memprof/TestCases/test_malloc_load_store.c
--
Exit Code: 1

Command Output (stderr):
--
clang-13: warning: treating 'c' input as 'c++' when in C++ mode, this behavior is deprecated [-Wdeprecated]
/usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/compiler-rt/test/memprof/TestCases/test_malloc_load_store.c:20:16: error: CHECK-NEXT: expected string not found in input
// CHECK-NEXT: #0 {{.*}} in malloc
               ^
<stdin>:37:2: note: scanning from here
 #0 0x44a71d in __interceptor_malloc /usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/compiler-rt/lib/memprof/memprof_malloc_linux.cpp:127:3
 ^
<stdin>:37:20: note: possible intended match here
 #0 0x44a71d in __interceptor_malloc /usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/compiler-rt/lib/memprof/memprof_malloc_linux.cpp:127:3
                   ^

Input file: <stdin>
Check file: /usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/compiler-rt/test/memprof/TestCases/test_malloc_load_store.c

-dump-input=help explains the following input dump.

Input was:
<<<<<<
           .
           .
           .
          32: Stack for id 1090519041:
          33:  #0 0x44a71d in __interceptor_malloc /usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/compiler-rt/lib/memprof/memprof_malloc_linux.cpp:127:3
          34:  #1 0x7fb446b078b0 in _dl_exception_create_format elf/dl-exception.c:146:21
          35: 
          36: Stack for id 1342177281:
          37:  #0 0x44a71d in __interceptor_malloc /usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/compiler-rt/lib/memprof/memprof_malloc_linux.cpp:127:3
next:20'0      X~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ error: no match found
next:20'1                        ?                                                                                                                                    possible intended match
          38:  #1 0x46a58d in main /usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/compiler-rt/test/memprof/TestCases/test_malloc_load_store.c:27:19
next:20'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          39:  #2 0x7fb4465b5d09 in __libc_start_main csu/../csu/libc-start.c:308:16
next:20'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
          40: 
next:20'0     ~
          41: Stack for id 1686110209:
next:20'0     ~~~~~~~~~~~~~~~~~~~~~~~~
          42:  #0 0x44a71d in __interceptor_malloc /usr/local/google/home/vitalybuka/src/llvm.git/llvm-project/compiler-rt/lib/memprof/memprof_malloc_linux.cpp:127:3
next:20'0     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
           .
           .
           .
>>>>>>

--

********************
********************
Failed Tests (1):
  MemProfiler-x86_64-linux :: TestCases/test_malloc_load_store.c

I cannot reproduce this error by ninja check-memprof from my sandbox.

This change, although related to StackDepot, does not use the one in compiler-rt/lib/sanitizer_common/sanitizer_stackdepot.cpp. It creates a new instance of StackDepotBase used by only Msan.