Naios / function2

Improved and configurable drop-in replacement to std::function that supports move only types, multiple overloads and more
http://naios.github.io/function2
Boost Software License 1.0
545 stars 46 forks source link

-Waddress warning generated for non-capturing lambdas on gcc <= 9.2 #48

Closed mystor closed 2 years ago

mystor commented 2 years ago

@Naios

In gcc versions before 9.3, the compiler would generate a -Waddress warning complaining that the address of a lambda's function will never be null if it is cast to a boolean (compiler explorer), even in generic code.

This impacts function2 because the generic logic to determine if a newly-constructed function object should be empty ends up triggering these warnings when passed a captureless lambda in various places, as it passes the use_bool_op checks:

https://github.com/Naios/function2/blob/035bec21533f484c1b5dd8dd0380109ea431f6a1/include/function2/function2.hpp#L1158 https://github.com/Naios/function2/blob/035bec21533f484c1b5dd8dd0380109ea431f6a1/include/function2/function2.hpp#L1220 https://github.com/Naios/function2/blob/035bec21533f484c1b5dd8dd0380109ea431f6a1/include/function2/function2.hpp#L1341

This can be seen with this example compiler explorer instance hitting the warning from each of these places: https://godbolt.org/z/vvKfonTa1

I can't think of a practical way off the top of my head to detect non-nullable captureless lambdas in use_bool_op and treat them differently than other function-like objects which can convert to bool, so the easiest approach may be to suppress the warning on these older compiler versions. It appears that libstdc++ just manually enumerates all types which can be checked for emptyness as required by the spec (https://github.com/gcc-mirror/gcc/blob/3b3f2f7c265ef9f176cb811a8049b24538d954d9/libstdc++-v3/include/bits/std_function.h#L210-L228) which I imagine isn't a practical approach for this library.


Commit Hash

02ca99831de59c7c3a4b834789260253cace0ced

Expected Behavior

No warning is emitted when building with -Waddress

Actual Behavior

-Waddress warnings are emitted on gcc 9.2 and earlier when constructing or assigning a function2 function object with a captureless lambda.

Steps to Reproduce

Use the function2 library with gcc 9.2 or earlier, enable -Waddress warnings, and construct a function2 function object with a captureless lambda.

Example godbolt instance: https://godbolt.org/z/vvKfonTa1

Your Environment

Naios commented 2 years ago

I'm actually not sure what is happening there. The library never calls the bool comparison if the object does not provide one. A lambda should never provide a comparison to bool, am I right?

The library already specializes this out, and the path of the comparison should never be taken in such cases see: https://github.com/Naios/function2/blob/035bec21533f484c1b5dd8dd0380109ea431f6a1/include/function2/function2.hpp#L1395.

Potentially GCC tries to pass the lamda as function pointer to the library.

mystor commented 2 years ago

Lambdas which don't capture any members do provide a comparison to bool through their implicit conversion to a function pointer (https://godbolt.org/z/Y484rq5c1)

int main() {
    // converts to a function pointer
    void(*foo)() = [](){};
    // converts to bool
    if ([](){}) {
    }

    int i = 0;
    // ERROR - doesn't convert to a function pointer
    void(*bar)() = [i](){};
    // ERROR - doesn't convert to bool
    if ([i](){}) { }
}

edit: link to the cppreference on the implicit conversion operator: https://en.cppreference.com/w/cpp/language/lambda#ClosureType::operator_ret.28.2A.29.28params.29.28.29

Naios commented 2 years ago

Well ok, I'm developing mainly with MSVC (and MSVC does not support this behaviour) so I was not aware of an implicit conversion to bool, thanks for your explanation.

I'm not sure how to fix this warning yet. I will take a look into it.

Btw. GCC's behaviour is inconsistent, using !! instead of bool(_) also triggers the warning in GCC 9.3.

https://godbolt.org/z/v6zxKac53

Naios commented 2 years ago

I started to implement a potential fix (https://github.com/Naios/function2/commit/4097e10b7e0bc60e9336c6b689e01612208fba79), however, I noticed that it is probably not possible to fix the warning without introducing potential edge cases with bad side effects. Basically, it is not possible to detect whether the given class is a lambda or a regular object implementing operator(). A regular class could still be implicitly convertible to a function pointer and therefore would break a potential detection that is based on the convertibility of a class to a function pointer, e.g. the following test would fail:

struct EvilObject {
  using Fn = int (*)();

  explicit operator bool() const {
    return false;
  }

  operator Fn() const {
    return +[] { return 0; };
  }

  int operator()() const {
    return 0;
  }
};

TEST(regression_tests, no_address_warning_in_constexpr_lambda_2) {
  fu2::function<int()>f(EvilObject{});

  ASSERT_FALSE(f); // Asserts here
}

Probably I'm only going to silence the warnings because performing the check (which probably is optimized out anyway) is less evil than implementing the said fix that could break something.

mystor commented 2 years ago

One other possibility which might be nicer would be to add a build option to have empty propagation modeled more after the way that std::function handles it, so instead of having it be based on whether it can implicitly coerce to bool it's based on a set of known types which should support empty pointers.

For this, it'd probably end up looking like this (https://godbolt.org/z/nE3MnPdzY):

template <typename T>
struct use_bool_op : std::false_type {};

template <typename Signature>
struct use_bool_op<std::function<Signature>> : std::true_type {};

template <typename Config, typename Property>
struct use_bool_op<function<Config, Property>> : std::true_type {};

template <typename T>
struct use_bool_op<T*> : std::true_type {};

template <typename Class, typename T>
struct use_bool_op<T Class::*> : std::true_type {};

This is roughly inspired by the wording of how the standard library checks for whether to propagate an empty state, based on an explicitly enumerated list of cases to consider (https://en.cppreference.com/w/cpp/utility/functional/function/function):

5) Initializes the target with std::forward<F>(f). The target is of type std::decay<F>::type. If f is a null pointer to function, a null pointer to member, or an empty value of some std::function specialization, *this will be empty after the call. [...]

basically just enumerates those cases and then adds an extra case for the internal fu2::detail::function type.

I imagine this would need to be added as a seperate configuration option to avoid breaking backcompat in case people are depending on other function types which implicitly coerce to bool.