Page MenuHomePhabricator

[tsan] Fix warnings in tsan_interceptors.cc from expansion of variadic macros
ClosedPublic

Authored by xiaobai on Oct 20 2017, 6:36 PM.

Details

Summary

C99 technically requires the rest arguments to be used in C variadic macros. This presents a
problem with the macro SCOPED_TSAN_INTERCEPTOR when func takes no arguments.
This happens with the function pause. Like other void argument functions, we pass in a fake argument to avoid this warning.

Event Timeline

xiaobai created this revision.Oct 20 2017, 6:36 PM
xiaobai edited the summary of this revision. (Show Details)Oct 20 2017, 6:36 PM
fjricci added inline comments.Oct 23 2017, 8:16 AM
lib/sanitizer_common/sanitizer_common_defs.h
16 ↗(On Diff #119732)

I think this should go in sanitizer_internal_defs.h

lib/tsan/rtl/tsan_interceptors.h
59 ↗(On Diff #119732)

Is it possible to solve this with ##__VA_ARGS__ instead? Not super familiar with how all of this stuff works though.

xiaobai added inline comments.Oct 23 2017, 10:06 AM
lib/tsan/rtl/tsan_interceptors.h
59 ↗(On Diff #119732)

Yes, that is one way to solve this. Unfortunately, that is a GNU extension, and is not considered portable. My solution should work on any platform correctly.

xiaobai updated this revision to Diff 119890.Oct 23 2017, 10:07 AM

Moving STRINGIFY macro to sanitier_internal_defs.h

fjricci added inline comments.Oct 23 2017, 10:12 AM
lib/tsan/rtl/tsan_interceptors.h
59 ↗(On Diff #119732)

Looks like clang supports it, and I believe that we don't support tsan on windows (so don't need to worry about MSVC). But I'll defer to other reviewers on the best way.

xiaobai added inline comments.Oct 23 2017, 3:29 PM
lib/tsan/rtl/tsan_interceptors.h
59 ↗(On Diff #119732)

Sure, I think it's totally fine to use the GNU extension. However, if supporting tsan on windows is something people want, this solution will probably be better.

dvyukov edited edge metadata.Oct 24 2017, 12:25 AM

Does the same requirement present in C++11?

This fix for a warning looks too complex to me.
I have hard time understanding how it works. I afraid if I even need to modify this code. It seems to be very subtly different from what we have now because ARGS_ZERO still expands to nothing.
I would prefer if we just do what we do for all other void interceptors -- append 'int fake' argument.

xiaobai edited the summary of this revision. (Show Details)Oct 24 2017, 5:57 PM
xiaobai updated this revision to Diff 120160.Oct 24 2017, 5:58 PM

Pass a fake argument instead of crafting complex variadic macros

I'm not sure if the same requirement is present in C++11.

I tried what you suggested based off of the other void argument functions that use a fake argument, which seems to work well here too. Much better than the complex macros! :)

dvyukov accepted this revision.Oct 24 2017, 10:34 PM
This revision is now accepted and ready to land.Oct 24 2017, 10:34 PM

I will commit this later today. Thanks.

dvyukov closed this revision.Oct 25 2017, 1:06 AM