This is an archive of the discontinued LLVM Phabricator instance.

[compiler-rt][hwasan] Refactor kAliasRegionStart usage
ClosedPublic

Authored by leonardchan on Jul 9 2021, 12:33 PM.

Details

Summary

This patch contains thre3 changes:

  • Add GetAliasRegionStart which will dispatch to either 0 or kAliasRegionStart depending on if aliasing mode is enabled.
  • Wrap instances of kAliasRegionStart so it's not emitted when aliasing mode is disabled.
  • Move kAliasRegionStart out of hwasan_linux.cpp so other platforms can reuse it if other platforms want to define a non-zero alias start.

For now, aliasing mode is not supported on fuchsia, so this takes care of "not using what we don't use" while making it easier to use if we do support it in the future.

Diff Detail

Event Timeline

leonardchan created this revision.Jul 9 2021, 12:33 PM
leonardchan requested review of this revision.Jul 9 2021, 12:33 PM
vitalybuka added inline comments.Jul 9 2021, 12:57 PM
compiler-rt/lib/hwasan/hwasan_mapping.h
51–53

can you hide this completly in cpp file and move GetAliasRegionStart into cpp as well?
This function rarely used, I don't think it's important to keep it inline

leonardchan added inline comments.Jul 9 2021, 1:39 PM
compiler-rt/lib/hwasan/hwasan_mapping.h
51–53

Something like this? I was hoping to prevent emitting an alias-related symbol when aliasing mode is disabled with the static inline similar to D104275.

vitalybuka added inline comments.Jul 9 2021, 3:33 PM
compiler-rt/lib/hwasan/hwasan_allocator.cpp
44

why here and not in linux where it's used?
I assumed you can make it in linux file
static uptr kAliasRegionStart;

compiler-rt/lib/hwasan/hwasan_linux.cpp
74–75

please clang format

As is I like "Diff 1" better then "Diff 2"

leonardchan added inline comments.Jul 12 2021, 10:39 AM
compiler-rt/lib/hwasan/hwasan_allocator.cpp
44

GetAliasRegionStart would still need to reference kAliasRegionStart in aliasing mode, so if it's in the linux file, then either GetAliasRegionStart is also defined there or we can define it elsewhere but kAliasRegionStart would still need to be extern in hwasan_mapping.h.

I believe the first case would mean GetAliasRegionStart would need to be re-implemented for other platforms, but I think it would be cleaner if we can just re-use GetAliasRegionStart (which can return 0) in non-aliasing mode. Currently, supporting a new platform requires defining kAliasRegionStart, but I think it would be cleaner if platforms that didn't use aliasing just don't need to define it at all.

vitalybuka accepted this revision.Jul 12 2021, 11:40 AM

either way LGTM

compiler-rt/lib/hwasan/hwasan_allocator.cpp
44

GetAliasRegionStart

85

can we just return here and remove corresponding stuff from linux file?
constexpr uptr kAliasRegionOffset = 1ULL << (kTaggableRegionCheckShift - 1);
return __hwasan_shadow_memory_dynamic_address + kAliasRegionOffset;

This revision is now accepted and ready to land.Jul 12 2021, 11:40 AM
vitalybuka added inline comments.Jul 12 2021, 11:41 AM
compiler-rt/lib/hwasan/hwasan_allocator.cpp
44

ignore this one

This revision was landed with ongoing or failed builds.Jul 12 2021, 4:35 PM
This revision was automatically updated to reflect the committed changes.
leonardchan marked 3 inline comments as done.