Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
67 ↗ | (On Diff #78238) | 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). |
123 ↗ | (On Diff #78238) | 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 | ||
---|---|---|
9 ↗ | (On Diff #80832) | Coverage dumper used with -fsanitize-coverage=tarce-pc-guards |
55 ↗ | (On Diff #80832) | I am confused. |
125 ↗ | (On Diff #80832) | Hm... Why? |
127 ↗ | (On Diff #80832) | use CHECK_NE |
140 ↗ | (On Diff #80832) | 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 | ||
6 ↗ | (On Diff #80832) | shouldn't you run this test in a separate scratch dir? |
10 ↗ | (On Diff #80832) | Mmm. really? I thought I've fixed it. |
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
9 ↗ | (On Diff #80832) | 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? |
55 ↗ | (On Diff #80832) | 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. |
125 ↗ | (On Diff #80832) | I have a check below in __sanitizer function. I'd prefer this detail to be handled over there. |
140 ↗ | (On Diff #80832) | 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. |
67 ↗ | (On Diff #78238) | see above suggestion how we can modify public API to be useful here. |
PTAL. Sancov test works, using only api functions now.
test/sanitizer_common/TestCases/sanitizer_coverage_trace_pc_guard.cc | ||
---|---|---|
10 ↗ | (On Diff #80832) | Ahh. Missing -1 :)) Works now. |
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
11 ↗ | (On Diff #80847) | remove comment |
64 ↗ | (On Diff #80847) | allow a special case of a zero pc (just ignore it) |
84 ↗ | (On Diff #80847) | is this one syscall per offset? |
96 ↗ | (On Diff #80847) | did you see my previous comment about DSOs? |
9 ↗ | (On Diff #80832) | file name is fine. |
test/sanitizer_common/TestCases/sanitizer_coverage_trace_pc_guard.cc | ||
23 ↗ | (On Diff #80847) | 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 | ||
---|---|---|
140 ↗ | (On Diff #80832) | what's wrong with sparse array? |
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
110 ↗ | (On Diff #80956) | We used exactly this approach in the previous implementation. |
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
118 ↗ | (On Diff #81135) | hm... Why i + end - start? The invariant here is that pc_vector.size() is max_guard_value + 1, |
122 ↗ | (On Diff #81135) | 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 | ||
---|---|---|
122 ↗ | (On Diff #81135) | memory_order_relaxed, of course |
atomic
lib/sanitizer_common/sanitizer_coverage_libcdep_new.cc | ||
---|---|---|
118 ↗ | (On Diff #81135) | 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 | ||
---|---|---|
118 ↗ | (On Diff #81135) | 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 | ||
---|---|---|
118 ↗ | (On Diff #81135) | 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 | ||
---|---|---|
118 ↗ | (On Diff #81135) |
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 | ||
---|---|---|
118 ↗ | (On Diff #81135) | 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. |