Page MenuHomePhabricator

hwasan: Untag global variable addresses in tests.
ClosedPublic

Authored by pcc on Aug 5 2019, 12:38 PM.

Details

Summary

Once we start instrumenting globals, all addresses including those of string literals
that we pass to the operating system will start being tagged. Since we can't rely
on the operating system to be able to cope with these addresses, we need to untag
them before passing them to the operating system. This change introduces a macro
that does so and uses it everywhere it is needed.

Diff Detail

Repository
rL LLVM

Event Timeline

pcc created this revision.Aug 5 2019, 12:38 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 5 2019, 12:38 PM
Herald added subscribers: Restricted Project, kubamracek. · View Herald Transcript
hctim added a comment.Aug 5 2019, 1:13 PM

Instead of putting UNTAG everywhere, could we change %clang_hwasan to instead have globals instrumentation disabled, then only use UNTAG in the hwasan_globals tests? WDYT?

pcc added a comment.Aug 5 2019, 1:22 PM

The reason why I did it this way was to make the tests as "realistic" as possible. The more we diverge from a "realistic" build the more likely it is that bugs slip through because we aren't testing what we ship.

hctim added a comment.Aug 5 2019, 1:37 PM

Understood. I'm not the biggest fan of putting macros all around the codebase - could we define our own ErrorPrintf()/Printf()/strcmp() and explicitly untag there? I think relying on explicit untagging everywhere may be error-prone, and it looks like we can reduce the mental overhead of remembering to explicitly untag.

pcc added a comment.Aug 5 2019, 1:45 PM

Understood. I'm not the biggest fan of putting macros all around the codebase - could we define our own ErrorPrintf()/Printf()/strcmp() and explicitly untag there? I think relying on explicit untagging everywhere may be error-prone, and it looks like we can reduce the mental overhead of remembering to explicitly untag.

If you forget to untag, your test will not pass once the global instrumentation change lands. So I'm personally not too concerned about this.

That said, adding the wrappers seems fine to me. I'll do that.

pcc updated this revision to Diff 213465.Aug 5 2019, 2:28 PM
  • Introduce wrappers
hctim accepted this revision.Aug 5 2019, 2:33 PM

LGTM with nits.

compiler-rt/test/hwasan/TestCases/utils.h
10 ↗(On Diff #213465)

Nit: > 80char

18 ↗(On Diff #213465)

Nit: > 80char

This revision is now accepted and ready to land.Aug 5 2019, 2:33 PM
pcc marked an inline comment as done.Aug 5 2019, 2:45 PM
pcc added inline comments.
compiler-rt/test/hwasan/TestCases/utils.h
10 ↗(On Diff #213465)

No idea why clang-format isn't obeying <=80 cols here, oh well.

This revision was automatically updated to reflect the committed changes.