This is an archive of the discontinued LLVM Phabricator instance.

[TSan] Refactor ExternalAccess() to avoid unnecessary pop/push tag [NFC]
ClosedPublic

Authored by yln on Mar 22 2023, 4:37 PM.

Details

Summary
  • Avoid unnecessary frame & tag push/pops if memory access is ignored
  • Rename function and add comment to make it clearer what the code does
  • Make helper functions static and move inside #if !SANITIZER_GO

Diff Detail

Event Timeline

yln created this revision.Mar 22 2023, 4:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2023, 4:37 PM
Herald added a subscriber: Enna1. · View Herald Transcript
yln requested review of this revision.Mar 22 2023, 4:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 22 2023, 4:37 PM
Herald added a subscriber: Restricted Project. · View Herald Transcript
yln retitled this revision from [TSan] Refactor ExternalAccess() to avoid unnecessary pop/push tag [NFC] r=dvyukov,kubamracek,wrotki to [TSan] Refactor ExternalAccess() to avoid unnecessary pop/push tag [NFC].Mar 22 2023, 4:38 PM
yln added a comment.Mar 22 2023, 4:44 PM

This the follow-up I promised here:
https://reviews.llvm.org/D146264#4202953

there is no space the shadow cells [...] for the uptr-sized tag

Is this still true with the new runtime? @dvyukov

The call that actually models the memory access always uses CALLERPC. Is this how it should be? @kubamracek

MemoryAccess(thr, tsan_caller_pc, (uptr)addr, 1, typ);

Here is the original code before my modifications:
https://reviews.llvm.org/D32360

dvyukov accepted this revision.Mar 22 2023, 11:37 PM
This revision is now accepted and ready to land.Mar 22 2023, 11:37 PM

Is this still true with the new runtime?

It's even true-er :)
The shadow was shrunk from 8 bytes to 4 bytes.

yln added a comment.Mar 24 2023, 11:12 AM

The call that actually models the memory access always uses CALLERPC. Is this how it should be?

Answering my own question: there is a test Darwin/external.cpp checking this, so I am assuming yes.