This is an archive of the discontinued LLVM Phabricator instance.

No -fsanitize=function warning when calling noexcept function through non-noexcept pointer in C++17
ClosedPublic

Authored by sberg on Dec 1 2017, 6:19 AM.

Details

Summary

As discussed in the mail thread https://groups.google.com/a/isocpp.org/forum/#!topic/std-discussion/T64_dW3WKUk "Calling noexcept function throug non-noexcept pointer is undefined behavior?", such a call should not be UB. However, Clang currently warns about it.

There is no cheap check whether two function type_infos only differ in noexcept, so pass those two type_infos as additional data to the function_type_mismatch handler (with the optimization of passing a null "static callee type" info when that is already noexcept, so the additional check can be avoided anyway). For the Itanium ABI (which appears to only record noexcept information for pointer-to-function type_infos, not for function type_infos themselves), we then need to check the mangled names for occurrence of "Do" representing "noexcept".

Diff Detail

Repository
rC Clang

Event Timeline

sberg created this revision.Dec 1 2017, 6:19 AM
sberg updated this revision to Diff 125152.Dec 1 2017, 8:36 AM

(Diff 125121 had accidentally contained a spurious "}". Fixed that now.)

sberg added a comment.Dec 13 2017, 3:42 AM

friendly ping

This revision was not accepted when it landed; it landed in state Needs Review.Dec 18 2017, 5:06 AM
This revision was automatically updated to reflect the committed changes.
sberg reopened this revision.Dec 18 2017, 5:59 AM

Had to revert r320977/r320978 again with r320981/r320982: "At least http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-android/builds/6013/steps/annotate/logs/stdio complains about
ubsan::ubsan_handle_function_type_mismatch_abort (compiler-rt lib/ubsan/ubsan_handlers.cc) returning now despite being declared 'noreturn', so looks like a different approach is needed for the function_type_mismatch check
to be called also in cases that may ultimately succeed."

Any hint how to achieve that properly?

vsk added a subscriber: vsk.Dec 18 2017, 10:21 AM
vsk added a comment.Dec 18 2017, 11:48 AM

Please add a test.

vsk added a comment.Dec 18 2017, 11:49 AM

Copying my comment from the diffusion thread to keep things in one place:

You could make FunctionTypeMismatch an 'AlwaysRecoverable' check, just like the Vptr check, and remove the _abort handlers from the ubsan runtimes.

In D40720#958677, @vsk wrote:

Please add a test.

Note that the bot upon the first closing of this review changed the shown diff from the combined cfe+compiler-rt diff to just the cfe part. See https://reviews.llvm.org/rL320977 for the compiler-rt part, including tests in compiler-rt/trunk/test/ubsan/TestCases/TypeCheck/Function/function.cpp.

pcc added a subscriber: pcc.Dec 18 2017, 1:29 PM

Would it be possible to fix this by stripping the noexcept specifiers from both the function type used in the check and the one that is embedded in the prefix data? The downside is that we won't catch the case where the caller has a noexcept specifier and the callee doesn't, but that seems like an edge case to me, and we can think about fixing it in other ways later.

vsk added a comment.Dec 18 2017, 1:37 PM
In D40720#958677, @vsk wrote:

Please add a test.

Note that the bot upon the first closing of this review changed the shown diff from the combined cfe+compiler-rt diff to just the cfe part. See https://reviews.llvm.org/rL320977 for the compiler-rt part, including tests in compiler-rt/trunk/test/ubsan/TestCases/TypeCheck/Function/function.cpp.

Ah sorry, I'd missed that. Still, it's always nice to have a test at the IR-gen level as well as the runtime test, since those can be a bit more stringent.

Would it be possible to fix this by stripping the noexcept specifiers from both the function type used in the check and the one that is embedded in the prefix data? The downside is that we won't catch the case where the caller has a noexcept specifier and the callee doesn't, but that seems like an edge case to me, and we can think about fixing it in other ways later.

This sounds fine to me, and it avoids breaking the trapping mode.

sberg added a comment.Dec 19 2017, 5:28 AM
In D40720#958743, @vsk wrote:

Would it be possible to fix this by stripping the noexcept specifiers from both the function type used in the check and the one that is embedded in the prefix data? The downside is that we won't catch the case where the caller has a noexcept specifier and the callee doesn't, but that seems like an edge case to me, and we can think about fixing it in other ways later.

This sounds fine to me, and it avoids breaking the trapping mode.

...and would be in line with what has been discussed in the mail sub-thread starting at http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20171218/213093.html "Re: r320982 - Revert r320978 'No -fsanitize=function warning when calling noexcept function through non-noexcept pointer in C++17'"

sberg updated this revision to Diff 127863.Dec 21 2017, 4:58 AM

As suggested, solve the issue instead by removing any "noexcept" from the typeinfo emitted for the -fsanitize=function checks.

sberg updated this revision to Diff 127899.Dec 21 2017, 8:55 AM

(need to call getAs<FunctionProtoType> instead of cast<FunctionProtoType> in one place, in case the name in the function decl is wrapped in parens, as happens in HarfBuzz's hb-buffer.cc)

rsmith added inline comments.Dec 21 2017, 10:36 AM
clang/lib/CodeGen/CodeGenTypes.h
378 ↗(On Diff #127899)

Please use the frontend language terminology here: "nothrow" is appropriate when talking about an LLVM function, but for the frontend type we should call this "removeExceptionSpecification" or similar. It would make sense for this to live on ASTContext instead of CodeGenTypes, since it's an AST type transformation utility not something related to IR generation. (There's a call to getFunctionType in Sema::IsFunctionConversion that you could also convert to use this ASTContext utility function.)

Perhaps exposing the currently-internal getFunctionTypeWithExceptionSpec from ASTContext.cpp would be a good way forward. (That function also handles wrapping, unwrapping, and adjusting type sugar nodes, but has the same effect as this one on the canonical type.)

clang/test/CodeGenCXX/ubsan-function-noexcept.cpp
5 ↗(On Diff #127899)

A name more meaningful than "LABEL" would be more useful to future readers of this code.

sberg updated this revision to Diff 128289.Dec 28 2017, 4:50 AM

made the recommended changes

sberg added a comment.Jan 4 2018, 4:58 AM

friendly ping

any further input, or should I consider this good enough to go in now?

rsmith accepted this revision.Jan 4 2018, 2:23 PM

LGTM, thank you!

This revision is now accepted and ready to land.Jan 4 2018, 2:23 PM
This revision was automatically updated to reflect the committed changes.
sberg added a comment.Jan 5 2018, 12:05 AM

Should this be backported to Clang 6? Not sure how widespread a problem this is in practice (it hit me with LibreOffice).

Herald added a project: Restricted Project. · View Herald TranscriptFeb 8 2019, 8:04 AM