Conversation
PhilMiller
left a comment
PhilMiller
left a comment
There was a problem hiding this comment.
As written, does whatever 'top-level' object that's rooting a serialization call get properly checked? I'm not quite sure on the interplay of the serializer, the dispatcher, and the push and pop calls.
|
Also, I looked over the corresponding bits in the clang tool, and they seem reasonable. |
|
There's a potential corner case of objects with empty-base optimizations in play, but that's so obscure as to likely be irrelevant. |
lifflander
force-pushed
the
98-serialize-checks
branch
from
02a5df4 to
04cb363
Compare
|
This side of things all looks good to me. |
lifflander
force-pushed
the
98-serialize-checks
branch
from
04cb363 to
f62c1f3
Compare
Codecov Report
@@ Coverage Diff @@
## develop #101 +/- ##
===========================================
- Coverage 93.76% 92.98% -0.78%
===========================================
Files 94 97 +3
Lines 2854 2909 +55
===========================================
+ Hits 2676 2705 +29
- Misses 178 204 +26
|
|
@PhilMiller @cz4rs Can you guys take a look at this? I've done a major overhaul of this code. I'm splitting up the concerns between the type-specific traversal for sanitization and the sanitizer runtime logic. Checkpoint will contain the typed overloads at all times. This means one may use a stock VT/Checkpoint and then enable the sanitizer later. The sanitizer runtime itself ( |
|
This won't be merged until the runtime is complete in the other repo and I confirm this is the correct interface that we need for that work properly. |
| /// pimpl to runtime that contains runtime sanitizer logic | ||
| extern Runtime* rt(); | ||
|
|
||
| /// function that informs sanitizer if its enabled |
|
|
||
| // A bit of a trick...recurse with a non-sanitizing-specific overload to | ||
| // resolve actual \c serialize methods without invoking the partial | ||
| // specialization |
There was a problem hiding this comment.
I think this comment may need more elaboration. I think I kinda get what's going on, but would prefer a full explanation.
|
|
||
| bool enabled() { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Would it make sense to have this be a member of the Runtime object?
There was a problem hiding this comment.
These have to be C functions because they are LD preloaded by the serializer runtime.
There was a problem hiding this comment.
Could you point me at where you picked up that constraint? That doesn't sound right to me, since LD_PRELOAD should just be importing whatever symbols it finds in a foo.so library into the list that ld.so will resolve against. If those are C function names or C++ mangled names shouldn't matter.
That said, this isn't a big deal.
|
Broadly, I think this looks very good. I'll be happy to see this tidied up and merged. |
| } | ||
|
|
||
| // Declare this member as serialized | ||
| checkpoint_sanitizer_rt()->isSerialized( |
There was a problem hiding this comment.
I think it would be preferable to do auto rt = checkpoint_sanitizer_tr() once here, and then just rt->method()
|
One important check - does this work if one doesn't include the Otherwise, I think this is ready to move forward, modulo my remarks about some comments and style. |
cz4rs
left a comment
cz4rs
left a comment
There was a problem hiding this comment.
looks good overall, I agree with Phil on "trick" part needing more explanation
3 participants
Fixes #98
Initial implémentation of stack-based checker using the clang tooling generated checks from:
https://github.com/DARMA-tasking/serialization-sanitizer