• Disallowed -fsanitize=function in combination with -fsanitize-minimal-runtime now.

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

friendly ping

friendly ping

friendly ping

friendly ping

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

Added missing tests at https://reviews.llvm.org/D61479 "Add tests for 'Adapt -fsanitize=function to SANITIZER_NON_UNIQUE_TYPEINFO'".

In D60760#1487342, @lebedev.ri wrote:

Did this get reviewed?

friendly ping

committed for now to get the crash fixed; if there are issues with the test they can be addressed later

The change itself should probably be uncontroversial (the bad cast had been there ever since getFunctionTypeWithExceptionSpec had been introduced with r221918), but I'm not sure about the test: It tests the relevant code somewhat indirectly; is it fine in clang/test/AST/?; or is such a test even overkill?

One problem I found with the macro __cpp_impl_destroying_delete not being conditional on language version is the following: Recent GCC trunk (since https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=76b94d4ba654e9af1882865933343d11f5c3b18b "Implement P0722R3, destroying operator delete.") contains

#if __cpp_impl_destroying_delete
#define __cpp_lib_destroying_delete 201806L
namespace std
{
  struct destroying_delete_t
  {
    explicit destroying_delete_t() = default;
  };
  inline constexpr destroying_delete_t destroying_delete{};
}
#endif // destroying delete

at "top-level" (i.e., not in a C++20-only #if or similar) in libstdc++-v3/libsupc++/new. That means that when using Clang against that GCC toolchain, #include <new> in C++03 mode will cause error: unknown type name 'constexpr'.

I run the new checker on LibreOffice project. I found ~25 false positives, which seems small enough to me. This false positives can be supressed easily.

I've silenced this scenario in r344898, thank you for raising the issue!

In D52400#1266341, @aaron.ballman wrote:

In D52400#1266307, @sberg wrote:

[...]

Then again, this is a case where you don't get any error but you do get a silent behavioral ambiguity without the current enumerator shadow diagnostic:

struct S1;
struct S2;
struct S3 {
  void S1();
  enum { S2 };

  void f(decltype(S2) s);
};

So there are cases where this behavior can be somewhat useful.

doesnt this make -Wshadow more aggressive for enumerators than for other entities?

see https://bugs.llvm.org/show_bug.cgi?id=37161 "clang-cl triggers ASTContext::getASTRecordLayout Assertion `D && 'Cannot get layout of forward declarations!''" for what appears to be fallout from this change

A random data point from trying this patch on the LibreOffice code base:

It looks like this caused a clang-cl regression https://bugs.llvm.org/show_bug.cgi?id=37146 "clang-cl emits special functions for non-trivial C-structs ('__destructor_8') introduced for Objective-C".

Turns out DLLAttr-inherited-from-class is only added to members during Sema::CheckCompletedClass -> Sema::checkClassLevelDLLAttribute, when friend re-decls of those members may already have been created.

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

friendly ping

made the recommended changes

(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)

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

added a small IR test

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.

In D40720#958677, @vsk wrote:

Please add a test.

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."

friendly ping

friendly ping; any input on my two questions maybe?

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

Oh, I'd meant to upload the version of the patch where the newly added calls to EmitTypeCheck use the default Alignment and SkippedChecks arguments, instead of explicitly skipping Alignment and ObjectSize checks. (That's the version I had successfully used in a LibreOffice UBSan 'make check' test build. But I still don't know which version is better.)

Some items I'm unclear about:

(This code had been introduced with http://llvm.org/viewvc/llvm-project?view=revision&revision=115028 "Define _Bool, bool, true, and false macros in <stdbool.h> when we're in a GNU-compatible C++ dialect. Fixes rdar://problem/8477819" mentioning: "Fixes rdar://problem/8477819." But I don't know what that is.)

In D36870#871740, @EricWF wrote:

Because libc++ doesn't actually have any instances of deleted virtual functions within the library __cxa_deleted_vilrtual is never used, and hence never exported on Linux.
We do, however, have pure virtual functions, so __cxa_pure_virtual is exported transitively. On Linux we don't use the explicit export lists, so this change didn't change the Linux ABI.

...but I'm still curious as to why lib/abi/x86_64-unknown-linux-gnu.abilist should want to mention __cxa_pure_virtual but not __cxa_deleted_virtual

Drop changes of files under lib/abi/<version>.

(the original diff had inadvertently lacked full -U999999 context; uploaded a new diff)

(the original diff had inadvertently lacked full -U999999 context; uploaded a new diff)