Page MenuHomePhabricator

[libc++] Allow assigning std::rethrow_if_nested() to a function pointer
Needs ReviewPublic

Authored by xingxue on Jul 21 2022, 8:53 AM.

Details

Reviewers
ldionne
Mordante
philnik
hubert.reinterpretcast
cebowleratibm
daltenty
Group Reviewers
Restricted Project
Summary

This patch fixes the error with assigning std::rethrow_if_nested() to a function pointer. The fix is similar to what is done for std::throw_with_nested().

The following test case demonstrates the issue.

#include <exception>

int main()
{
  struct A {};
  void (*p)(const A&) = &std::rethrow_if_nested<A>;
}

Diff Detail

Unit TestsFailed

TimeTest
1,990 mslibcxx CI Clang-cl (DLL) > llvm-libc++-shared-clangcl-cfg-in.std/language_support/support_exception/except_nested::call_thru_funcptr.pass.cpp
Script: -- : 'COMPILED WITH'; 'C:/Program Files/LLVM/bin/clang-cl.exe' C:\ws\w5\llvm-project\libcxx-ci\libcxx\test\std\language.support\support.exception\except.nested\call_thru_funcptr.pass.cpp --driver-mode=g++ --target=x86_64-pc-windows-msvc -nostdinc++ -I C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1 -I C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/include/c++/v1 -I C:/ws/w5/llvm-project/libcxx-ci/libcxx/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -DTEST_WINDOWS_DLL -nostdlib -L C:/ws/w5/llvm-project/libcxx-ci/build/clang-cl-dll/lib -lc++ -lmsvcrt -lmsvcprt -loldnames -o C:\ws\w5\llvm-project\libcxx-ci\build\clang-cl-dll\test\std\language.support\support.exception\except.nested\Output\call_thru_funcptr.pass.cpp.dir\t.tmp.exe
1,190 mslibcxx CI Clang-cl (Static) > llvm-libc++-static-clangcl-cfg-in.std/language_support/support_exception/except_nested::call_thru_funcptr.pass.cpp
Script: -- : 'COMPILED WITH'; 'C:/Program Files/LLVM/bin/clang-cl.exe' C:\ws\w3\llvm-project\libcxx-ci\libcxx\test\std\language.support\support.exception\except.nested\call_thru_funcptr.pass.cpp --driver-mode=g++ --target=x86_64-pc-windows-msvc -nostdinc++ -I C:/ws/w3/llvm-project/libcxx-ci/build/clang-cl-static/include/c++/v1 -I C:/ws/w3/llvm-project/libcxx-ci/build/clang-cl-static/include/c++/v1 -I C:/ws/w3/llvm-project/libcxx-ci/libcxx/test/support -D_CRT_SECURE_NO_WARNINGS -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_STDIO_ISO_WIDE_SPECIFIERS -DNOMINMAX -std=c++2b -Werror -Wall -Wextra -Wshadow -Wundef -Wno-unused-command-line-argument -Wno-attributes -Wno-pessimizing-move -Wno-c++11-extensions -Wno-noexcept-type -Wno-atomic-alignment -Wno-user-defined-literals -Wno-tautological-compare -Wsign-compare -Wunused-variable -Wunused-parameter -Wunreachable-code -Wno-unused-local-typedef -D_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER -D_LIBCPP_ENABLE_EXPERIMENTAL -D_LIBCPP_DISABLE_AVAILABILITY -fcoroutines-ts -Werror=thread-safety -Wuser-defined-warnings -llibc++experimental -nostdlib -L C:/ws/w3/llvm-project/libcxx-ci/build/clang-cl-static/lib -llibc++ -lmsvcrt -lmsvcprt -loldnames -o C:\ws\w3\llvm-project\libcxx-ci\build\clang-cl-static\test\std\language.support\support.exception\except.nested\Output\call_thru_funcptr.pass.cpp.dir\t.tmp.exe

Event Timeline

xingxue created this revision.Jul 21 2022, 8:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 8:53 AM
xingxue requested review of this revision.Jul 21 2022, 8:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 21 2022, 8:53 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript

I'm not too familiar with this code so I won't approve it. I wonder whether we want to allow taking the address of this function.

libcxx/test/std/language.support/support.exception/except.nested/call_thru_funcptr.pass.cpp
7

please don't use thru in the filename, use the complete word.

35

Some of our supported platforms require this.

39

Is there wording in the Standard allowing this?

xingxue updated this revision to Diff 446869.Jul 22 2022, 9:22 AM

Addressed comments.

  • Renamed test case to call_through_funcptr.pass.cpp;
  • Define main as 'main(int, char**)`;
  • Formatting changes.

I'm not too familiar with this code so I won't approve it. I wonder whether we want to allow taking the address of this function.

From this document namespace.std, it seems it is allowed to take the address of std::rethrow_if_nested. @ldionne, @hubert.reinterpretcast, please correct me if my interpretation is wrong. Thanks!

Let F denote a standard library function ([global.functions]), a standard library static member function, or an instantiation of a standard library function template.
Unless F is designated an addressable function, the behavior of a C++ program is unspecified (possibly ill-formed) if it explicitly or implicitly attempts to form a pointer to F.
[Note 1: Possible means of forming such pointers include application of the unary & operator ([expr.unary.op]), addressof ([specialized.addressof]), or a function-to-pointer standard conversion ([conv.func]).
— end note]
Moreover, the behavior of a C++ program is unspecified (possibly ill-formed) if it attempts to form a reference to F or if it attempts to form a pointer-to-member designating either a standard library non-static member function ([member.functions]) or an instantiation of a standard library member function template.

I'm not too familiar with this code so I won't approve it. I wonder whether we want to allow taking the address of this function.

From this document namespace.std, it seems it is allowed to take the address of std::rethrow_if_nested. @ldionne, @hubert.reinterpretcast, please correct me if my interpretation is wrong. Thanks!

Let F denote a standard library function ([global.functions]), a standard library static member function, or an instantiation of a standard library function template.
Unless F is designated an addressable function, the behavior of a C++ program is unspecified (possibly ill-formed) if it explicitly or implicitly attempts to form a pointer to F.
[Note 1: Possible means of forming such pointers include application of the unary & operator ([expr.unary.op]), addressof ([specialized.addressof]), or a function-to-pointer standard conversion ([conv.func]).
— end note]
Moreover, the behavior of a C++ program is unspecified (possibly ill-formed) if it attempts to form a reference to F or if it attempts to form a pointer-to-member designating either a standard library non-static member function ([member.functions]) or an instantiation of a standard library member function template.

Did I overlook something and is std::rethrow_if_nested marked as addressable?

Did I overlook something and is std::rethrow_if_nested marked as addressable?

Hi @Mordante, Thanks for your comments. I searched the standards and could not find any thing that is marked as addressable. I can be wrong but my reasoning is that rethrow_if_nested() is a global function that has a physical implementation and a physical address. Therefore, it is addressable. The following code snippet and 'nm' output show it has a physical address, noting that the signature of rethrow_if_nested() has __enable_if etc. as an argument and that is the reason why it does not match the signature defined by the standard, i.e., void rethrow_if_nested(const E&), when assigned to a function pointer. throw_with_nested() used to have the same problem but was fixed. This patch takes the same approach as used for throw_with_nested().

$ cat t.cpp:
#include <exception>
int main(int, char**)
{
    int i=5;
    std::rethrow_if_nested(i);
    return 0;
}
$ nm t.o
0000000000000000 T main
                 U .TOC.
0000000000000000 W _ZSt17rethrow_if_nestedB6v15000IiEvRKT_PNSt3__19enable_ifIXntsr18__can_dynamic_castIS0_St16nested_exceptionEE5valueEvE4typeE

Did I overlook something and is std::rethrow_if_nested marked as addressable?

Hi @Mordante, Thanks for your comments. I searched the standards and could not find any thing that is marked as addressable.

These should be some things marked, IIRC mainly in <iostream>.

I can be wrong but my reasoning is that rethrow_if_nested() is a global function that has a physical implementation and a physical address. Therefore, it is addressable. The following code snippet and 'nm' output show it has a physical address, noting that the signature of rethrow_if_nested() has __enable_if etc. as an argument and that is the reason why it does not match the signature defined by the standard, i.e., void rethrow_if_nested(const E&), when assigned to a function pointer. throw_with_nested() used to have the same problem but was fixed. This patch takes the same approach as used for throw_with_nested().

True, it's addressable now. But this isn't guaranteed by the Standard, hence the question do we want to guarantee this works with libc++. If we do we probably should document that as an extension. Since this isn't Standard behaviour the test should be moved from std to libcxx since this might fail on other implementations.

But let's wait and see what @ldionne thinks about adding this guarantee.

Hi @ldionne, What do you think about this? The affected test case works with GCC.