This is an archive of the discontinued LLVM Phabricator instance.

[dfsan] Add utils to get and print origin paths and some test cases
ClosedPublic

Authored by stephan.yichao.zhao on Mar 4 2021, 8:57 AM.

Diff Detail

Event Timeline

stephan.yichao.zhao requested review of this revision.Mar 4 2021, 8:57 AM
stephan.yichao.zhao created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptMar 4 2021, 8:57 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
gbalats added inline comments.Mar 4 2021, 11:49 AM
compiler-rt/include/sanitizer/dfsan_interface.h
121

Is this printed to stdout? Should we introduce an output stream argument instead to make it more versatile?

compiler-rt/lib/dfsan/dfsan.cpp
794

This is not very clear. It might be mistaken to mean that origin tracking is not supported. Also, it doesn't indicate to the user what they are doing wrong.

794

Should this be an error perhaps?

801
826–828

Is there a test for this code path?

845

If the loop is not entered at all, should the return value be 0 (now) or origin?

compiler-rt/test/dfsan/origin_ldst.c
32

Why not typedefs instead?

compiler-rt/test/dfsan/origin_limit.c
31

You can use CHECK-COUNT here to remove duplication.

stephan.yichao.zhao marked 3 inline comments as done.Mar 4 2021, 3:54 PM
stephan.yichao.zhao added inline comments.
compiler-rt/include/sanitizer/dfsan_interface.h
121

Agree that would be better.

dfsan_print_origin_trace eventually uses stack.Print() by sanitizer's Printf to print to stderr.
Printf is safe to use inside sanitizer code because high-level stream may not work.

If we assume this API is always called from customer code, I feel another stream can work if we can change stack.Print() works with a configurable output.
Other sanitizers print traces in the shared code path. so that needs a separate work.

I changed the comments to mention 'stderr'.

compiler-rt/lib/dfsan/dfsan.cpp
794

Since we print all the messages to stderr, changed to use colors (d.Warning()) for this kind of messages.

801

Also set the warning color.

826–828

Added a test case by using asm to bypass DFSan instrumentation.
Also added test to cover other corner cases.

845

That would be 0.
If along a chain, an invalid origin is found because of unknown issues: like somehow origin memory was overwritten, 0 also returns.
When a chain reaches to the very beginning, it also returns 0.
So we are able to distinguish the two cases. Maybe we can improve this later.

But the print out stack trace can tell which we get a valid chain, because a real very origin should be an stack about dfsan_set_label.

compiler-rt/test/dfsan/origin_ldst.c
32

Thank you.

stephan.yichao.zhao marked 3 inline comments as done.Mar 4 2021, 3:55 PM
stephan.yichao.zhao added inline comments.
compiler-rt/test/dfsan/origin_limit.c
31

Thank you.

stephan.yichao.zhao marked an inline comment as done.

addressed comments

gbalats added inline comments.Mar 4 2021, 4:12 PM
compiler-rt/lib/dfsan/dfsan.cpp
851–858

Iiuc, raw_id is initialized to 0 so we could place origin_id var outside the loop so that we don't have to check the condition twice.

simplified the loop in dfsan_get_init_origin

stephan.yichao.zhao marked an inline comment as done.Mar 4 2021, 4:18 PM
stephan.yichao.zhao added inline comments.
compiler-rt/lib/dfsan/dfsan.cpp
851–858

Thank you. This simplified the code.

gbalats accepted this revision.Mar 4 2021, 4:42 PM
This revision is now accepted and ready to land.Mar 4 2021, 4:42 PM
morehouse accepted this revision.Mar 5 2021, 9:23 AM
morehouse added inline comments.
compiler-rt/include/sanitizer/dfsan_interface.h
124

At some point we may want an API to traverse the origin chain. But this is fine for now.

compiler-rt/lib/dfsan/dfsan.cpp
711

Do we also need a dfsw wrapper?

802–803
810–811
813

For addr, do we need %p?

831–833
842–843
847–848
compiler-rt/test/dfsan/origin_add_label.c
20

Why do we need this cast?

stephan.yichao.zhao marked 10 inline comments as done.

addressed comments.

LLVM testing does not have debug info by default.
Added a dfsan config with debug info on to test stack traces with file/line information.

compiler-rt/lib/dfsan/dfsan.cpp
711

When origin_tracking is off, we do not generate wrappers with origin arguments.

813

yes. %p works perfectly than using 0x%x. Thank you.

compiler-rt/test/dfsan/origin_add_label.c
20

Thank you. we only need cast at the next line to shift 4 bytes. This one is not useful.

This revision was landed with ongoing or failed builds.Mar 5 2021, 4:12 PM
This revision was automatically updated to reflect the committed changes.