Page MenuHomePhabricator

Finish "Adapt -fsanitize=function to SANITIZER_NON_UNIQUE_TYPEINFO"

Authored by sberg on May 2 2019, 11:55 PM.



i.e., recent 5745eccef54ddd3caca278d1d292a88b2281528b:

  • Bump the function_type_mismatch handler version, as its signature has changed.
  • The function_type_mismatch handler can return successfully now, so SanitizerKind::Function must be AlwaysRecoverable (like for SanitizerKind::Vptr).
  • But the minimal runtime still unconditionally treats a call to the function_type_mismatch handler as failure, so document that. (An alternative would be to disallow -fsanitize=function in combination with -fsanitize-minimal-runtime, like it is done for -fsanitize=vptr.)
  • Add tests.

Diff Detail


Event Timeline

sberg created this revision.May 2 2019, 11:55 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptMay 2 2019, 11:55 PM
Herald added subscribers: llvm-commits, Restricted Project, kubamracek. · View Herald Transcript

clang codegen tests missing.
The problem with such things is that it got landed without review, and with zero tests.
And now you've got a feature that has no test coverage (== may have gotten broken in meantime), and a stuck patch that might add tests for it..

sberg updated this revision to Diff 200493.May 21 2019, 6:58 AM
sberg retitled this revision from Add tests for "Adapt -fsanitize=function to SANITIZER_NON_UNIQUE_TYPEINFO" to Finish "Adapt -fsanitize=function to SANITIZER_NON_UNIQUE_TYPEINFO".
sberg edited the summary of this revision. (Show Details)

Of course, adding missing tests reveals shortcomings in the new code.

sberg added a comment.Jun 4 2019, 12:25 AM

friendly ping

sberg added a comment.Jun 11 2019, 1:18 AM

friendly ping

sberg added a comment.Jun 18 2019, 8:01 AM

friendly ping

friendly ping

Should I land this to ensure a consistent state for upcoming LLVM 9?

sberg added a project: Restricted Project.Jun 24 2019, 12:07 PM

Any thoughts on this? (cfe-commits had inadvertently been missing from subscribers, it touches clang as well as compiler-rt.)

vsk added a comment.Jul 11 2019, 10:56 AM

This looks reasonable to me. I'd prefer to disallow -fsanitize=function in combination with -fsanitize-minimal-runtime, as I'm not sure it's appropriate for the minimal runtime to handle SANITIZER_NON_UNIQUE_TYPEINFO incorrectly. If this has already been discussed elsewhere I'd appreciate a pointer.

I have a more minor concern about the complexity of the ubsan test's RUN lines and \#include magic. I'm worried that these factors could make the test brittle (w.r.t unexpected directory structures or platforms that don't support $(...) expressions). But I don't have a clearly superior alternative to suggest. One idea is to a) redefine sanitizer_non_unique_typeinfo in the lit config and b) define %clangxx to include a path to the sanitizer_common headers. That might make the test easier to read, but not less brittle. Do you see this as a problem and if so are there any alternatives you might suggest?

eugenis added inline comments.Jul 11 2019, 11:21 AM
207 ↗(On Diff #200493)

I don't think this is true. As far as I can see, __ubsan_handle_function_type_mismatch in the minimal runtime simply prints a report and exits, i.e. -fsanitize=function is incompatible and should be rejected by the driver.

sberg updated this revision to Diff 209865.Jul 15 2019, 7:57 AM
  • Disallowed -fsanitize=function in combination with -fsanitize-minimal-runtime now.
  • Left the ubsan test's RUN lines and \#include magic as-is, if there's no clearly better way to avoid this unfortunate complexity.
vsk added a comment.Jul 15 2019, 12:30 PM

Looks reasonable to me -- mind waiting for another +1 on this to be safe? @eugenis, any thoughts?

eugenis accepted this revision.Jul 15 2019, 12:41 PM

LGTM, but please update the summary:

An alternative would be to disallow -fsanitize=function ...
This revision is now accepted and ready to land.Jul 15 2019, 12:41 PM
This revision was automatically updated to reflect the committed changes.

eh, the summary here doesn't get updated from the actual git commit message :(

thanks for the reviews!

eh, the summary here doesn't get updated from the actual git commit message :(

thanks for the reviews!

You can use arc diff --verbatim to update the summary based on your local commit message. You'll want to have done an arc amend before that to get any extra reviewers, subscribers, etc. from Phabricator into your local commit message, otherwise the arc diff --verbatim will happily override those too.