This is an archive of the discontinued LLVM Phabricator instance.

[memprof] Make the raw binary format the default.
ClosedPublic

Authored by snehasish on Nov 10 2021, 2:40 PM.

Details

Summary

Set the default memprof serialization format as binary. For the tests,
we update the configuration to include 'print_text=true'.

Diff Detail

Event Timeline

snehasish requested review of this revision.Nov 10 2021, 2:40 PM
snehasish created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2021, 2:40 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript

How many tests would be affected by switching the default?

Half of the tests (9/18) fail if the default is set to the binary format.

Failed Tests (9):

MemProfiler-x86_64-linux :: TestCases/dump_process_map.cpp
MemProfiler-x86_64-linux :: TestCases/log_path_test.cpp
MemProfiler-x86_64-linux :: TestCases/memprof_merge_mib.cpp
MemProfiler-x86_64-linux :: TestCases/memprof_profile_dump.cpp
MemProfiler-x86_64-linux :: TestCases/test_malloc_load_store.c
MemProfiler-x86_64-linux :: TestCases/test_memintrin.cpp
MemProfiler-x86_64-linux :: TestCases/test_new_load_store.cpp
MemProfiler-x86_64-linux :: TestCases/test_terse.cpp
MemProfiler-x86_64-linux :: TestCases/unaligned_loads_and_stores.cpp

Half of the tests (9/18) fail if the default is set to the binary format.

Failed Tests (9):

MemProfiler-x86_64-linux :: TestCases/dump_process_map.cpp
MemProfiler-x86_64-linux :: TestCases/log_path_test.cpp
MemProfiler-x86_64-linux :: TestCases/memprof_merge_mib.cpp
MemProfiler-x86_64-linux :: TestCases/memprof_profile_dump.cpp
MemProfiler-x86_64-linux :: TestCases/test_malloc_load_store.c
MemProfiler-x86_64-linux :: TestCases/test_memintrin.cpp
MemProfiler-x86_64-linux :: TestCases/test_new_load_store.cpp
MemProfiler-x86_64-linux :: TestCases/test_terse.cpp
MemProfiler-x86_64-linux :: TestCases/unaligned_loads_and_stores.cpp

IMO it's probably better to be explicit in the test - e.g. we are already setting log_path=stderr for all of these I believe.

snehasish updated this revision to Diff 386557.Nov 11 2021, 9:50 AM

Update tests to specify print_text=true instead of setting as default.

snehasish updated this revision to Diff 386559.Nov 11 2021, 9:52 AM

Update comment.

Half of the tests (9/18) fail if the default is set to the binary format.

Failed Tests (9):

MemProfiler-x86_64-linux :: TestCases/dump_process_map.cpp
MemProfiler-x86_64-linux :: TestCases/log_path_test.cpp
MemProfiler-x86_64-linux :: TestCases/memprof_merge_mib.cpp
MemProfiler-x86_64-linux :: TestCases/memprof_profile_dump.cpp
MemProfiler-x86_64-linux :: TestCases/test_malloc_load_store.c
MemProfiler-x86_64-linux :: TestCases/test_memintrin.cpp
MemProfiler-x86_64-linux :: TestCases/test_new_load_store.cpp
MemProfiler-x86_64-linux :: TestCases/test_terse.cpp
MemProfiler-x86_64-linux :: TestCases/unaligned_loads_and_stores.cpp

IMO it's probably better to be explicit in the test - e.g. we are already setting log_path=stderr for all of these I believe.

Done. PTAL, thanks!

tejohnson added inline comments.Nov 11 2021, 9:54 AM
compiler-rt/test/memprof/lit.cfg.py
35

I noticed that the other sanitizers (e.g. llvm/llvm-project/compiler-rt/test/asan/lit.cfg.py) add the trailing ":" after setting e.g. ASAN_OPTIONS. Will we end up with an extra trailing ":" for that env var?

snehasish updated this revision to Diff 386565.Nov 11 2021, 9:58 AM

Add the trailing separator only if the var is set.

This revision is now accepted and ready to land.Nov 11 2021, 9:59 AM
snehasish marked an inline comment as done.Nov 11 2021, 10:31 AM
snehasish added inline comments.
compiler-rt/test/memprof/lit.cfg.py
35

Yes, thats a little bit cleaner though we could still end up with a trailing ':' if the test does not provide any options. Since these do not affect option parsing, I'm happy to update this to be consistent with the ASAN config.

snehasish marked an inline comment as done.

Rebase.

I can't reproduce the patch application failure on the buildbot. I applied the patches in series to a clean llvm repo and it worked.

This revision was landed with ongoing or failed builds.Nov 11 2021, 11:31 AM
This revision was automatically updated to reflect the committed changes.