This is an archive of the discontinued LLVM Phabricator instance.

[dfsan] Add origin ABI wrappers for thread/signal/fork
ClosedPublic

Authored by stephan.yichao.zhao on Mar 10 2021, 9:06 AM.

Diff Detail

Event Timeline

stephan.yichao.zhao requested review of this revision.Mar 10 2021, 9:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2021, 9:06 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript

disabled testing on non-x86-64 CPUs

morehouse added inline comments.Mar 11 2021, 4:06 PM
compiler-rt/lib/dfsan/dfsan_thread.cpp
74

Please fix these lints.

compiler-rt/test/dfsan/origin_atomic.cpp
46 ↗(On Diff #330061)

This test is identical to atomic.cpp. Let's just add a RUN line there instead.

compiler-rt/test/dfsan/origin_custom.cpp
1 ↗(On Diff #330061)

It seems like a shame to duplicate so much code.

Can we somehow combine this test with custom.cpp, possibly with ifdefs for the origin stuff?

compiler-rt/test/dfsan/origin_fork.cpp
8 ↗(On Diff #330061)

Do we need to specify c++11?

29 ↗(On Diff #330061)

Since we aren't testing MSan, can we remove all uses of unintialized memory from this test?

compiler-rt/test/dfsan/origin_pthread.c
1 ↗(On Diff #330061)

Can we combine this test with pthread.c?

compiler-rt/test/dfsan/origin_signal_stress_test.cpp
1 ↗(On Diff #330061)

Can we combine this test with sigaction_stress_test.c?

stephan.yichao.zhao marked 6 inline comments as done.Mar 11 2021, 6:15 PM
stephan.yichao.zhao added inline comments.
compiler-rt/lib/dfsan/dfsan_thread.cpp
74

This is the same issue we had before, the web-based lint requires a different format from the local lint from arc.

compiler-rt/test/dfsan/origin_custom.cpp
1 ↗(On Diff #330061)

Use ORIGIN_TRACKING to control which calls are ready to test to prevent link errors.

compiler-rt/test/dfsan/origin_fork.cpp
8 ↗(On Diff #330061)

removed

29 ↗(On Diff #330061)

set to 0.

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

merged test cases to dedup same code.

morehouse added inline comments.Mar 12 2021, 9:20 AM
compiler-rt/test/dfsan/atomic.cpp
33

Should we also check origins here?

compiler-rt/test/dfsan/custom.cpp
149

This macro saves origins for each object val. However the previous ones save origins for each byte of val. Is this intentional?

280
943
953

Why do we zero the origin for the return value, but not for set?

If we only check origins when labels are nonzero, do we need to touch origin at all when we zero labels?

1288
1441
compiler-rt/test/dfsan/fork.cpp
28
40
compiler-rt/test/dfsan/origin_with_sigactions.c
2

Please add a brief comment explaining why we don't track origins in sig handlers.

8

Why MSan flags? Also below..

40

If we make these two handlers call a helper function, we can share their FileCheck lines below.

44

Are all the (void *) casts needed?

compiler-rt/test/dfsan/origin_with_signals.cpp
37

Isn't this test superfluous since we have origin_with_sigactions.cpp?

compiler-rt/test/dfsan/pthread.c
26

Are the casts needed?

33

Is the cast needed?

63

CHECK8 and CHECK16 look the same. Why do we need to test both?

stephan.yichao.zhao marked 14 inline comments as done.Mar 12 2021, 10:35 AM
stephan.yichao.zhao added inline comments.
compiler-rt/test/dfsan/custom.cpp
149

Yea. One could be removed... Since this change does not use DEFINE_AND_SAVE_N_ORIGINS, I removed it, and will double-check it in the following changes that need it.

953

Thank you. Removed unnecessary zero-out at origin return from dfsan_custom.cpp and all related tests.

compiler-rt/test/dfsan/origin_with_sigactions.c
8

fixed.

40

I did not use line numbers because we use a -NOT pattern to ensure no origins from signal handlers are recorded.
If we insert code later, using accurate line numbers would also fail -NOT.

Changed the two functions to share code.

44

Removed volatile. Then the cast is not necessary.

compiler-rt/test/dfsan/origin_with_signals.cpp
37

origin_with_sigactions tests only sigaction. This one tests signal. In dfsan_custom.cpp they have different code path, so we added this test.

compiler-rt/test/dfsan/pthread.c
26

removed. also removed other similar casts.

33

we do not need this after removing volatile.

63

I added those because if each thread prints a trace, all traces are mixed.
But actually printing from one thread is fine.
Thank you,

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

updated

morehouse added inline comments.Mar 12 2021, 3:13 PM
compiler-rt/lib/dfsan/dfsan_custom.cpp
536–538
917–918
1042–1044
1082–1083
1100–1101
compiler-rt/test/dfsan/atomic.cpp
45

Don't we need dfsan_get_origin? I think this is just setting origin = 10.

compiler-rt/test/dfsan/origin_with_sigactions.c
83

Now that we have CopyXtoYtoU, looks like the CHECK_HANDLE and CHECK_ACTION lines are identical. Let's share them.

stephan.yichao.zhao marked 7 inline comments as done.Mar 12 2021, 3:27 PM
stephan.yichao.zhao added inline comments.
compiler-rt/test/dfsan/atomic.cpp
45

Yes. It must be dfsan_get_origin. Thank you for catching this.

compiler-rt/test/dfsan/origin_with_sigactions.c
83

merged.

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

addressed comments

This revision is now accepted and ready to land.Mar 12 2021, 3:44 PM
This revision was landed with ongoing or failed builds.Mar 15 2021, 9:18 AM
This revision was automatically updated to reflect the committed changes.