This is an archive of the discontinued LLVM Phabricator instance.

tsan: deduplicate mapping selection
ClosedPublic

Authored by dvyukov on Aug 9 2021, 12:49 AM.

Details

Summary

Currently we have mapping selection duplicated 9 times.
Deduplicate it. No functional changes.

Depends on D107734.

Diff Detail

Event Timeline

dvyukov requested review of this revision.Aug 9 2021, 12:49 AM
dvyukov created this revision.
Herald added a project: Restricted Project. · View Herald TranscriptAug 9 2021, 12:49 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
melver accepted this revision.Aug 9 2021, 3:51 AM
melver added inline comments.
compiler-rt/lib/tsan/rtl/tsan_platform.h
599

If MappingType was moved above this function, then this could also use 'MappingType arg.?

664

The wrapping of Apply into MappingField is only required because of Result, right?

You could make SelectMapping have 'auto' return, then this wouldn't be needed.

666

Maybe I missed something, but why is this not using 'MappingType type'? Then the compiler would also check the switch for exhaustiveness.
Similarly all other Apply implementations.

This revision is now accepted and ready to land.Aug 9 2021, 3:51 AM
melver added inline comments.Aug 9 2021, 8:55 AM
compiler-rt/lib/tsan/rtl/tsan_platform.h
666

( And in case it is applicable, perhaps it's also an opportunity to switch to an 'enum class'. )

dvyukov updated this revision to Diff 365210.Aug 9 2021, 10:21 AM

use auto as return type

dvyukov added inline comments.Aug 9 2021, 10:22 AM
compiler-rt/lib/tsan/rtl/tsan_platform.h
599

This SelectMapping function is now generalized to call caller-passed function and it replaces _9_ different copy-pastes of the same code that were different by the called function.
Most of these functions accept uptr (addresses), only one version accepted MappingType. I could parametrize the arg type as typename Func::Arg but I decided it's not worth it.

664

Not only because of return type.
We want to parametrize SelectMapping with effectively a function template. I.e. pass a function that is itself parameterized by Mapping.
There are no function template template arguments, only classes can be template template arguments. The only way I know to do it is to wrap the function in a class, thus Func::Apply.

But using 'auto' for return type is indeed a good idea and it allows to get rid of Result typedef.
I've changed the code to use auto.

666

I've tried this one (the rest inherently want uptr arg type) and it gives me warnings (intentional) and an error (not intentional). I am not sure how to avoid the error w/o introducing another typedef for Func::Arg.

tsan_platform.h:667:13: warning: enumeration values 'MAPPING_APP_BEG' and 'MAPPING_APP_END' not handled in switch [-Wswitch]

switch (type) {
        ^

tsan_platform.h:637:10: error: no matching function for call to 'Apply'

return Func::template Apply<Mapping>(arg);
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~

tsan_platform.h:699:10: note: in instantiation of function template specialization 'tsan::SelectMapping<tsan::MappingField>' requested here

return SelectMapping<MappingField>(MAPPING_LO_APP_BEG);
       ^

tsan_platform.h:666:17: note: candidate function template not viable: no known conversion from 'sanitizer::uptr' (aka 'unsigned long') to 'tsan::MappingType' for 1st argument

static Result Apply(MappingType type) {
dvyukov added inline comments.Aug 9 2021, 10:25 AM
compiler-rt/lib/tsan/rtl/tsan_platform.h
666

We can cast uptr to the enum _within_ the function to get warnings about missed switch cases.
Lemme do this separately because it will cause lots of rebase conflicts and also current we have litigate warnings about missed switch cases because of these #ifdef's.

melver added inline comments.Aug 9 2021, 10:32 AM
compiler-rt/lib/tsan/rtl/tsan_platform.h
666

Re error: If it's just passing through the type to Apply then another 'typename Arg' on SelectMapping would be just as safe.

But re warnings, if intentional, then uptr is probably ok (or have a 'default:' but that defeats the the compiler checking things for us).

dvyukov added inline comments.Aug 9 2021, 10:36 AM
compiler-rt/lib/tsan/rtl/tsan_platform.h
666

The warnings are semi-intentional. They actually should be fixed by subsequent commits that unify names and remove ifdefs.

What do you think re:
What do you think re casting 'uptr' to MappingType _within_ the function so that we still get warnings about missed switch cases; and introducing enum class; but keeping arg type as uptr?
But I would like to do it in a new commit on top of this series.

dvyukov updated this revision to Diff 365236.Aug 9 2021, 11:45 AM

changed SelectMapping to use template Arg and rebased

dvyukov added inline comments.Aug 9 2021, 11:59 AM
compiler-rt/lib/tsan/rtl/tsan_platform.h
666

I've changed SelectMapping arg to "template Arg" and used MappingType enum as arg here. PTAL.

melver accepted this revision.Aug 9 2021, 12:08 PM

Glad it worked.

This revision was automatically updated to reflect the committed changes.