This is an archive of the discontinued LLVM Phabricator instance.

[tsan] Add debugging interfaces into interface header.
ClosedPublic

Authored by pgousseau on Mar 31 2023, 9:41 AM.

Diff Detail

Event Timeline

pgousseau created this revision.Mar 31 2023, 9:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2023, 9:41 AM
Herald added a subscriber: Enna1. · View Herald Transcript
pgousseau requested review of this revision.Mar 31 2023, 9:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 31 2023, 9:41 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Remove incorrect check

vitalybuka added inline comments.Mar 31 2023, 11:45 AM
compiler-rt/include/sanitizer/tsan_interface.h
18

we don't have tsan for windows

21

common_interface_defs includes stdint.h
maybe just use uintptr_t?

pgousseau updated this revision to Diff 510451.Apr 3 2023, 4:27 AM

Change uptr to uintptr_t
Fix doxygen return.

pgousseau marked 2 inline comments as done.Apr 3 2023, 4:29 AM
pgousseau added inline comments.
compiler-rt/include/sanitizer/tsan_interface.h
21

Yes makes sense, fixed now thanks!

@dvyukov This LGTM, do you have any concerns exposing this API?

compiler-rt/include/sanitizer/tsan_interface.h
181

Would you like in a separate patch to update compiler-rt/include/sanitizer to use doxygen style?

dvyukov accepted this revision.Apr 6 2023, 12:13 AM

@dvyukov This LGTM, do you have any concerns exposing this API?

They are already used by Apple debugger, so in some sense they are already exposed and the info we provide with reports did not change in the past 10 years, so I think it's OK.
If we won't have some of that data in future, we can just start returning errors, or null pointers from these functions.

This revision is now accepted and ready to land.Apr 6 2023, 12:13 AM
pgousseau marked 2 inline comments as done.Apr 11 2023, 3:00 AM

Thank you for the review!

compiler-rt/include/sanitizer/tsan_interface.h
181

Yes we would be interested in adding doxygen style comments to the whole tsan header at least so future api's comments will also hopefully be using doxygen. Will put in my TODOs, thanks!

This revision was landed with ongoing or failed builds.Apr 12 2023, 3:03 AM
This revision was automatically updated to reflect the committed changes.
pgousseau marked an inline comment as done.
hans added a subscriber: hans.Apr 12 2023, 11:31 AM

This broke the tsan tests on Darwin, see e.g. the end of https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket/8784015461553130001/+/u/package_clang/stdout

It seems those tests have their own declarations of some of these functions, which mismatch slightly.

Sorry about that, thank you for reverting. I will update the patch with hopefully better types.