Details
Diff Detail
- Build Status
Buildable 2080 Build 2080: arc lint + arc unit
Event Timeline
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
68 | I don't think this code belongs here. As I've mentioned before, we need to implement this using a public interface (which needs to be implemented). We already have sanitizer_symbolize_pc which can give you the module and the offset, Symbolizer::FindModuleForAddress does what you need (or almost). | |
124 | avoid comment like this, if possible. |
I frankly don't remember your arguments and so I don't remember why (and if) I agreed.
Can you repeat the arguments?
[please reply in phab, it doesn't properly line up the replies done in e-mail :( ]
I already need this API in libFuzzer.
The API could be simple, as described above (sanitizer_get_module_and_offset_for_pc())
then let's design and implement it in a separate CL.
Yes. And test.
you wanted trace-pc-guard deployed as soon as possible.
Yes, but that not a good reason to duplicate the code, and the patch partially duplicates the existing code in sanitizer_common.
To do what you are asking me to do would require changing the way LoadedModules stores the information.
Probably yes.
Also, I suggest to implement the API w/o changing the internals, test it and submit.
Then see if it needs performance improvements.
Kostya,
Please take another look. This is now fully functional with test passing.
I can't use __sanitizer_get_module_and_offset_for_pc because it returns char* module_name, and organizing lookup by name is problematic.
please add a test with at least one (better, two) DSOs
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
10 | Coverage dumper used with -fsanitize-coverage=tarce-pc-guards | |
56 | I am confused. | |
126 | Hm... Why? | |
128 | use CHECK_NE | |
141 | technically speaking, this does not prevent us from a race of another sort: BTW, if something needs to be atomic, that's the *guard I had another scheme in mind (and in the docs):
| |
test/sanitizer_common/TestCases/sanitizer_coverage_trace_pc_guard.cc | ||
7 | shouldn't you run this test in a separate scratch dir? | |
11 | Mmm. really? I thought I've fixed it. |
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
10 | I'll have to rename the file then. I told me this will be the basis for all sanitizer coverage implementations in the future. Is this sanitizer-coverage_libcdep_tracepcguard.cc? | |
56 | That's why I didn't want to create it in the first place. It is useless for me right now. I suggest we change it to return module base address, not the 'pc - base'. This way I will be able to identify module by base address. | |
68 | see above suggestion how we can modify public API to be useful here. | |
126 | I have a check below in __sanitizer function. I'd prefer this detail to be handled over there. | |
141 | this would create sparse pc_array. Not sure if we want this. Besides the code now is dead-simple. I like it this way. You are right that sometimes we can get same PC twice, but I don't think we really care about this race. What's the problem of having some PC listed twice in .sancov file? Looks harmless. |
PTAL. Sancov test works, using only api functions now.
test/sanitizer_common/TestCases/sanitizer_coverage_trace_pc_guard.cc | ||
---|---|---|
11 | Ahh. Missing -1 :)) Works now. |
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
10 | file name is fine. | |
12 | remove comment | |
65 | allow a special case of a zero pc (just ignore it) | |
85 | is this one syscall per offset? | |
97 | did you see my previous comment about DSOs? | |
test/sanitizer_common/TestCases/sanitizer_coverage_trace_pc_guard.cc | ||
24 | I thought I added a commend asking to extend the test to have DSOs, but I can't find it now. |
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
141 | what's wrong with sparse array? |
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
111 | We used exactly this approach in the previous implementation. |
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
119 | hm... Why i + end - start? The invariant here is that pc_vector.size() is max_guard_value + 1, | |
123 | Let's do proper atomics here. idx = atomic_exchange(guard, 0, memory_order_acquire); if (!idx) return; .... |
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
123 | memory_order_relaxed, of course |
atomic
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
119 | obviously. The line was before the loop, I moved it below and forgot to fix it. Done. |
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
119 | Still hmmmm. and just doing pc_vector.resize(i+1) won't work too, because next time you call it you will do u32 i = pc_vector.size(); and so new 'i' will be old_i+1, which is not what you want. I would decouple the counter and the vector size and just have another data member in TracePcGuardController for (u32* p = start; p < end; p++) *p = ++num_guards; |
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
119 | I think this is exactly what I need. Let's say we call it twice. First time len=2, second time len=3.
|
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
119 |
But that's wrong! :) pc_vector[idx] = pc; where idx is either 1 or 2. |
using idx - 1
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
119 | I see what you mean. It's actually correct to do pc_vector[idx - 1] = pc, since we start idx from 1. Fixed. I'll try to trigger it in the test. |
Coverage dumper used with -fsanitize-coverage=tarce-pc-guards
(or some such)