Resurrection of accidentally closed http://reviews.llvm.org/D7172
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
sorry for delay...
lib/sanitizer_common/sanitizer_common.cc | ||
---|---|---|
58–65 | What's "pname"? Can this be more descriptive? | |
304 | does binary name change over time? | |
lib/sanitizer_common/sanitizer_linux.cc | ||
709–710 | I am lost here. What's being changed? | |
lib/sanitizer_common/sanitizer_printf.cc | ||
257 | So, now everyone will have the module name in their logs by default? | |
test/asan/lit.cfg | ||
135 ↗ | (On Diff #19139) | I think this is an overkill. Just make the tests require linux/Freebsd |
lib/sanitizer_common/sanitizer_common.cc | ||
---|---|---|
58–65 | Ok. | |
304 |
You can mess with process name at least on some platforms. I'm not sure /proc/self/exe can be changed though.
You mean initialize it in _asan_init, _msan_init, etc.? Ok. | |
lib/sanitizer_common/sanitizer_linux.cc | ||
709–710 | I removed the redundant 4. | |
lib/sanitizer_common/sanitizer_printf.cc | ||
257 | I could but is there a good reason not to make this default (besides legacy scripts)? |
You mean initialize it in _asan_init, _msan_init, etc.? Ok.
Yes, create something like InitializeBinaryName() and GetBinaryName(), GetWhateverElseName() in sanitizer_common and call it in tool init routines.
I could but is there a good reason not to make this default (besides legacy scripts)?
legacy scripts is a good reason by itself, but more importantly, you are the first to request this in 4 years,
which means that most users don't need it. If they don't need it, extra bloat in the report will be a distraction.
If we see that the feature is useful we can always set the flag on by default.
but more importantly, you are the first to request this in 4 years,
That's probably because not many people sanitize full distributions.
lib/asan/asan_rtl.cc | ||
---|---|---|
311 | empty line? | |
314 | do you really need to call this twice? | |
lib/sanitizer_common/sanitizer_common.cc | ||
357 | So why do you still need the Mutex if you are calling this only once at init time? | |
lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc | ||
647 | So, do we still need this here? | |
test/asan/TestCases/verbose-log-path_test.cc | ||
9 | maybe just put it inside test/asan/TestCases/Linux? | |
16 | this test could be smaller, e.g. return glob[argc * 10]; } |
lib/asan/asan_rtl.cc | ||
---|---|---|
314 | Yes, just like AsanCheckIncompatibleRT is called twice in two types of initializers that we have (__asan_init and AsanInitializer). But - I should move it to AsanInitializer::AsanInitializer(). | |
lib/sanitizer_common/sanitizer_common.cc | ||
357 | I thought about that but I decided to keep CacheBinaryName more generic so that it could be used in other contexts besides pure program startup. I could remove this if you think it's unnecessary. | |
lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc | ||
647 | I don't know the whole sandboxing thing well enough so I kept it. | |
test/asan/TestCases/verbose-log-path_test.cc | ||
9 | Well, it's absolutely trivial to support on other platforms so I thought it makes sense to simply xfail. | |
16 | That's a blind copy-paste of similar log_path test without log_exe_name. |
lib/sanitizer_common/sanitizer_common.cc | ||
---|---|---|
357 | Yes, I'd prefer to simplify it and make sure it crashes if called twice, or while other threads are running (or at least after the init is done) | |
lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc | ||
647 | This should be safe, especially if the tests pass :) | |
test/asan/TestCases/verbose-log-path_test.cc | ||
16 | Sure. Please make this one simpler. |
Updated based on previous review. Unfortunately I was unable to prohibit against multiple calls to CheckBinaryName because both UBSan and ASan call it.
lib/sanitizer_common/sanitizer_common.cc | ||
---|---|---|
343 | initialize to nullptr, so that if we forget to call CacheBinaryName we instantly crash | |
356 | This will not catch a situation where someone calls CacheBinaryName() twice, which should be illegal. | |
360 | You can probably implement this thing w/o an extra static bool, just use binary_basename_cache_str |
lib/sanitizer_common/sanitizer_common.cc | ||
---|---|---|
343 | Yeah, I thought about this. But note that even though we call CacheBinaryName very early, some code has to execute before it. And I'd prefer this code to print something at least to stderr instead of crashing. | |
356 | Unfortunately we do have situations where double call is needed e.g. when app is both A- and UB-sanitized. | |
360 | Right. |
lib/sanitizer_common/sanitizer_common.cc | ||
---|---|---|
343 | Sorry, I still don't understand - how do you propose to handle failing CHECKs in e.g. InitializeFlags (which is called before CacheBinaryName has a chance to set binary_basename_cache_str)? If we CHECK_NE(binary_basename_cache_str, nullptr) we'll abort without reporting the real error. |
lib/sanitizer_common/sanitizer_common.cc | ||
---|---|---|
343 | Can you then read the binary name earlier? |
lib/sanitizer_common/sanitizer_common.cc | ||
---|---|---|
356 | Could you be more specific? I can see that ASan and UBSan have been changed to have isolated copies of some core structures (e.g. suppressions). But error reporting (e.g. report_file or log_path) are still shared across all sanitizers with mutexes being used to serialize accesses. |
lib/sanitizer_common/sanitizer_common.cc | ||
---|---|---|
356 | Alexey actually tells that this part will be ready later this week. |
Sure, thanks for the update. Could you guys make sure that this works properly with dynamic A- and UB-runtimes in GCC (which have separate sanitizer_commons)?
OMG. I'll let Alexey speak here, but if I were him I would not support this mode.
What we should have is
- separate UBSan run-time
- bundled ASan+UBSan run-time
Having two sanitizers co-exist in the same process w/o properly knowing about each other is beyond of what we can reasonably maintain.
Looks mostly good. I'd rather land this in three changes: (1) refactor Asan dynamic initialization (2) move CacheBinaryName to sanitizer_common and call it early (3) implement log_exe_name.
lib/asan/asan_linux.cc | ||
---|---|---|
116 ↗ | (On Diff #23491) | just if (!ASAN_DYNAMIC) return; |
lib/asan/asan_rtl.cc | ||
325 | I think that moving these functions to the AsanInitInternal is a positive change, do you think you can land it independently (as a refactoring), before submitting the rest of this patch? | |
lib/sanitizer_common/sanitizer_common.cc | ||
228–231 | That's weird, don't we handle backslash on Windows above? | |
lib/sanitizer_common/sanitizer_common.h | ||
211 | It is already declared below. | |
lib/sanitizer_common/sanitizer_printf.cc | ||
261 | You should probably check that needed_length is not greater than buffer_size here as well. | |
lib/sanitizer_common/sanitizer_symbolizer_posix_libcdep.cc | ||
648 | Note that moving CacheBinaryName() to sanitizer_common (and calling it early during initialization) may also be separated to an independent change, that shouldn't change user-visible behavior. | |
lib/tsan/rtl/tsan_rtl.cc | ||
309 | I'd rather do this after setting SanitizerToolName and parsing the flags, as you do in different sanitizers. | |
test/asan/TestCases/verbose-log-path_test.cc | ||
2 | Consider changing the name of the output executable (%t-verbose-log-path_test-binary)... | |
7 | ... and matching against this executable name here. | |
10 | XFAIL: win32,android,darwin |
Once again, sorry for delay :-/
Np, this isn't the most interesting change for sure.
lib/asan/asan_rtl.cc | ||
---|---|---|
325 | Sure. | |
lib/sanitizer_common/sanitizer_common.cc | ||
228–231 | True. | |
lib/sanitizer_common/sanitizer_printf.cc | ||
261 | Good catch, otherwise there may be an overflow. | |
lib/tsan/rtl/tsan_rtl.cc | ||
309 | Will do. | |
test/asan/TestCases/verbose-log-path_test.cc | ||
7 | Sorry, not quite clear. What would be the benefit of this? |
test/asan/TestCases/verbose-log-path_test.cc | ||
---|---|---|
7 | Otherwise you seem to depend on the fact that %t (temporary string generated by lit) contains the source file name (verbose-log-path_test.cc). |
Updated based on previous review + split independent functionality to separate patches.
lib/sanitizer_common/sanitizer_printf.cc | ||
---|---|---|
260 | Hm, now this chunk of code is copied three times in the function... Factor it out to a macro, maybe? |
empty line?