This is an archive of the discontinued LLVM Phabricator instance.

Make detect_invalid_pointer_pairs option to be tristate.
ClosedPublic

Authored by marxin on Dec 20 2017, 11:29 PM.

Diff Detail

Repository
rL LLVM

Event Timeline

marxin created this revision.Dec 20 2017, 11:29 PM

Is !nullptr expected to be special cased forever or it's transitional?

lib/asan/asan_flags.inc
141 ↗(On Diff #127831)

Can we change it to match detect_odr_violation style?

int, detect_invalid_pointer_pairs, 0,
"If >= 2, detect operations like <, <=, >, >= and - on invalid pointer "
"pairs (e.g. when pointers belong to different objects); "
"If == 1, detect invalid operations only when both pointers are non-null.")
lib/asan/asan_report.cc
350 ↗(On Diff #127831)
switch (flags()->detect_invalid_pointer_pairs) {
  case 0 : return;
  case 1 : if (p1 == nullptr || p2 == nullptr) return; break;
}
marxin updated this revision to Diff 131206.Jan 24 2018, 1:43 AM

Update version where I addressed comments.
The tristate is not a transition, but it's design to have more fine control of the ASAN check.

marxin marked 2 inline comments as done.Jan 24 2018, 1:44 AM
alekseyshl accepted this revision.Jan 24 2018, 7:05 PM
alekseyshl added inline comments.
lib/asan/asan_report.cc
346 ↗(On Diff #131206)

Well, now you don't need this check.

Also, move a1 and a2 declarations past switch statement.

This revision is now accepted and ready to land.Jan 24 2018, 7:05 PM
marxin updated this revision to Diff 131395.Jan 25 2018, 1:16 AM

Updated version that addresses the comment.

marxin marked an inline comment as done.Jan 25 2018, 1:16 AM

Fixed, can you please install the patch?

alekseyshl added inline comments.Jan 25 2018, 11:19 AM
lib/asan/asan_report.cc
346 ↗(On Diff #131395)

Again, you do not need this check anymore, it's a part of switch statement now.

marxin updated this revision to Diff 131543.Jan 25 2018, 11:57 PM
marxin marked an inline comment as done.

Comments fixed, please install it if you're fine with that.

lib/asan/asan_report.cc
346 ↗(On Diff #131395)

No, sorry for overlooking that.

This revision was automatically updated to reflect the committed changes.