Enable -Wstrict-aliasing warning by bstaletic · Pull Request #2816 · pybind/pybind11

bstaletic · 2021-01-23T20:11:04Z

Description

Even though our cmake uses -fno-strict-aliasing, not all extensions do. Since C++17, we can actually fix -Wstrict-aliasing errors. For now, let's just see how things fail in CI.

Suggested changelog entry:

bstaletic

Previous run, which errored: https://github.com/pybind/pybind11/pull/2816/checks?check_run_id=1755015877

bstaletic · 2021-01-23T20:27:49Z

+#if defined(__has_include) && __has_include(<version>)
+# include <version>
+#endif


Since C++20, __cpp_lib_foo feature test macros are in <version> header and at least clang respects that.

bstaletic · 2021-01-23T20:28:26Z

+#if defined(__cpp_lib_launder) && !(defined(_MSC_VER) && (_MSC_VER < 1914))
+# define PYBIND11_STD_LAUNDER std::launder
+# define PYBIND11_HAS_STD_LAUNDER 1
+#else
+# define PYBIND11_STD_LAUNDER
+# define PYBIND11_HAS_STD_LAUNDER 0
+#endif
+


This is the best check that @YannickJadoul and I could come up with.

Like I mentioned in the other comment, __cpp_lib_launder will be defined in <version> in C++20. If I define these macros here, I won't get that header from detail/common.h.
Should I include <version> here? Move these macro definitions to common.h?
Also defined(__cpp_lib_launder) should be defined(__cpp_lib_launder) && __cpp_lib_launder >= 201606.

bstaletic · 2021-01-23T20:29:25Z

+# if !PYBIND11_HAS_STD_LAUNDER
+# pragma GCC diagnostic ignored "-Wstrict-aliasing"
+# endif


In case we don't have std::launder, we can't tackle -Wstrict-aliasin. At least not easily, without breaking or a major refactoring.

bstaletic · 2021-01-23T20:29:53Z

 #endif
 if (!std::is_trivially_destructible<Func>::value)
- rec->free_data = [](function_record *r) { ((capture *) &r->data)->~capture(); };
+ rec->free_data = [](function_record *r) { PYBIND11_STD_LAUNDER((capture *) &r->data)->~capture(); };


As far as I know, this is the only place where we need to launder.

YannickJadoul

I wish there was a way to avoid std::launder (and UB on C++<17), but ... myeah, it's better than before, at least.

YannickJadoul · 2021-01-23T22:13:17Z

+#if defined(__cpp_lib_launder) && !(defined(_MSC_VER) && (_MSC_VER < 1914))
+# define PYBIND11_STD_LAUNDER std::launder
+# define PYBIND11_HAS_STD_LAUNDER 1
+#else
+# define PYBIND11_STD_LAUNDER
+# define PYBIND11_HAS_STD_LAUNDER 0


Oh, I'd still move this up (underneath detail/common.h) having been included?

YannickJadoul · 2021-01-23T22:38:02Z

 #if defined(__GNUG__) && !defined(__clang__) && __GNUC__ >= 6
 # pragma GCC diagnostic pop
 #endif
+#if !PYBIND11_HAS_STD_LAUNDER


Suggested change
#if !PYBIND11_HAS_STD_LAUNDER
#ifdefined(__GNUG__) &&!PYBIND11_HAS_STD_LAUNDER
or
Suggested change
#if !PYBIND11_HAS_STD_LAUNDER
#ifdefined(__GNUG__) && !defined(__clang__) &&!PYBIND11_HAS_STD_LAUNDER

YannickJadoul · 2021-01-24T00:41:05Z

I was not expecting that to work, but that message seems to have been received...
Well, it's Saturday evening. Let's not judge MSVC for having had one too many?

…'s pybind#2816)

@bstaletic

#2819) * When determining if a shared_ptr already exists, use a test on the weak_ptr instead of a try/catch block. * When determining if a shared_ptr already exists, use a test on the weak_ptr instead of a try/catch block. * weak_from_this is only available in C++17 and later * Switch to use feature flag instead of C++ version flag. * Add Microsoft-specific check. * Avoid undefined preprocessor macro warning treated as error. * Simplify shared_from_this in init_holder * Include <version> in detail/common.h (~stolen~ borrowed from @bstaletic's #2816) * Move class_::get_shared_from_this to detail::try_get_shared_from_this * Simplify try_get_shared_from_this by using weak_ptr::lock() Co-authored-by: Yannick Jadoul <yannick.jadoul@belgacom.net>

bstaletic · 2021-07-14T14:19:48Z

I think I got this to work on icc as well.

henryiii

Always happy to see these ignores go away (even if only conditionally)! And become more specific to the code they are there for.

I'm wondering if things like missing-field-initializers can go away after the last few PRs fixing clang-tidy?

Skylion007 · 2021-07-14T17:04:57Z

Always happy to see these ignores go away (even if only conditionally)! And become more specific to the code they are there for.
I'm wondering if things like missing-field-initializers can go away after the last few PRs fixing clang-tidy?

Not quite, I can enable that clang-tidy check too.

bstaletic · 2021-07-14T20:58:53Z

I'm wondering if things like missing-field-initializers can go away after the last few PRs fixing clang-tidy?

I tried and gcc 11 did not complain. However, this is surprising:

In function ‘cast’,
inlined from ‘operator()’ at /home/bstaletic/work/pybind11/include/pybind11/pybind11.h:213:43,
inlined from ‘_FUN’ at /home/bstaletic/work/pybind11/include/pybind11/pybind11.h:191:21:
/home/bstaletic/work/pybind11/include/pybind11/cast.h:567:13: warning: ‘operator delete’ called on unallocated object ‘int_string_pair’ [-Wfree-nonheap-object]
567 | delete src;
| ^
/home/bstaletic/work/pybind11/include/pybind11/cast.h: In function ‘_FUN’:
/home/bstaletic/work/pybind11/tests/test_builtin_casters.cpp:174:40: note: declared here
174 | static std::pair<int, std::string> int_string_pair{2, "items"};
| ^
In function ‘cast’,
inlined from ‘operator()’ at /home/bstaletic/work/pybind11/include/pybind11/pybind11.h:213:43,
inlined from ‘_FUN’ at /home/bstaletic/work/pybind11/include/pybind11/pybind11.h:191:21:
/home/bstaletic/work/pybind11/include/pybind11/stl.h:186:5: warning: ‘operator delete’ called on unallocated object ‘lvv’ [-Wfree-nonheap-object]
186 | PYBIND11_TYPE_CASTER(Type, _("List[") + value_conv::name + _("]"));
| ^
/home/bstaletic/work/pybind11/include/pybind11/stl.h: In function ‘_FUN’:
/home/bstaletic/work/pybind11/tests/test_stl.cpp:79:38: note: declared here
79 | static std::vector<RValueCaster> lvv{2};
| ^

rwgk

The pragma block at the top of pybind11.h is evil (I lost a few hours already because of this), nice to see at least one item go.

Side remark: The repetition of a complex #if for the pragma ... pop seems unfortunate (DRY) but is common practice. Might be nice to go through at some point to do something like this:

#if something complicated
# pragma ...
# define PYBIND11_PRAGMA_ACTIVE 1
#endif
...
#if defined(PYBIND11_PRAGMA_ACTIVE)
# pragma ...
# undef PYBIND11_PRAGMA_ACTIVE
#endif

rwgk · 2021-07-15T13:58:42Z

I'm wondering if things like missing-field-initializers can go away after the last few PRs fixing clang-tidy?
I tried and gcc 11 did not complain. However, this is surprising:

None of our sanitizers picks up on this. I think they would if it was real.

…andard_layout`) https://chatgpt.com/share/67ea07fa-9afc-8008-95af-4d7e0829a9c2 Apply std::launder consistently to functional capture storage This commit improves the robustness and clarity of pybind11's internal capture mechanism for bound C++ callables stored in function_record. - Introduces `capture::from_data(void**)` to encapsulate reinterpretation of the in-place `capture` object stored in `rec->data`, ensuring consistent use of `std::launder`. - Adds a `static_assert` that verifies `capture` is standard layout, making explicit the precondition necessary for these low-level performance optimizations. - Centralizes the `PYBIND11_STD_LAUNDER` and `PYBIND11_HAS_STD_LAUNDER` macros into `common.h` to enable reuse across headers. - Replaces ad hoc laundering and strict aliasing warnings with a more principled and maintainable approach. This refines and generalizes the use of `std::launder` introduced in PR pybind#2816 (2021-07-15), addressing undefined behavior more thoroughly while preserving ABI and performance characteristics.

bstaletic force-pushed the strict-aliasing branch from 9576122 to 6154bf5Compare January 23, 2021 20:26

bstaletic commented Jan 23, 2021
View reviewed changes

bstaletic force-pushed the strict-aliasing branch from 6154bf5 to d68f534Compare January 23, 2021 20:46

YannickJadoul approved these changes Jan 23, 2021
View reviewed changes

YannickJadoul reviewed Jan 23, 2021
View reviewed changes

bstaletic force-pushed the strict-aliasing branch from fe06e0a to 0e4e1c6Compare January 23, 2021 22:30

YannickJadoul reviewed Jan 23, 2021
View reviewed changes

bstaletic force-pushed the strict-aliasing branch 4 times, most recently from e7225e6 to 257af0eCompare January 23, 2021 23:50

bstaletic commented Jan 24, 2021
View reviewed changes

Comment threadinclude/pybind11/pybind11.h Outdated

YannickJadoul force-pushed the strict-aliasing branch from e6ddb66 to 98bae49Compare January 24, 2021 00:32

bstaletic force-pushed the strict-aliasing branch from 98bae49 to ca1e08dCompare January 25, 2021 22:28

YannickJadoul added a commit to elkhrt/pybind11 that referenced this pull request Jan 30, 2021
Include <version> in detail/common.h (~stolen~ borrowed from@bstaletic…
ac9dd31
…'s pybind#2816)

YannickJadoul added a commit to elkhrt/pybind11 that referenced this pull request Jan 30, 2021
Include <version> in detail/common.h (~stolen~ borrowed from@bstaletic…
f8b1406
…'s pybind#2816)

bstaleticand others added 3 commits February 7, 2021 23:17

Enable -Wstrict-aliasing warning
4b4c43c

Narrow down the scope of -Wstrict-aliasing
d4ca9d4

Go home, MSVC, you're drunk
1578e69

bstaletic force-pushed the strict-aliasing branch from ca1e08d to 1578e69Compare February 7, 2021 22:18

henryiii closed this Jul 13, 2021

henryiii reopened this Jul 13, 2021

Make sure "pragma GCC" is not executed on ICC
7b2e7be

henryiii approved these changes Jul 14, 2021
View reviewed changes

rwgk approved these changes Jul 15, 2021
View reviewed changes

Skylion007 approved these changes Jul 15, 2021
View reviewed changes

Skylion007 merged commit 5cd3750 into pybind:masterJul 15, 2021

github-actionsBot added the needs changelog Possibly needs a changelog entry label Jul 15, 2021

henryiii removed the needs changelog Possibly needs a changelog entry label Jul 15, 2021

bstaletic deleted the strict-aliasing branch July 16, 2021 10:13

rwgk mentioned this pull request Feb 10, 2023
FWD pybind11 google/pybind11clif#2816
Closed

rwgk mentioned this pull request Mar 31, 2025
function_record code health enhancements (std::launder, std::is_standard_layout) #5592
Merged

	#if !PYBIND11_HAS_STD_LAUNDER
	#ifdefined(__GNUG__) &&!PYBIND11_HAS_STD_LAUNDER

	#if !PYBIND11_HAS_STD_LAUNDER
	#ifdefined(__GNUG__) && !defined(__clang__) &&!PYBIND11_HAS_STD_LAUNDER

Uh oh!

Conversation

bstaletic commented Jan 23, 2021

Description

Suggested changelog entry:

Uh oh!

bstaletic left a comment

Choose a reason for hiding this comment

Uh oh!

bstaleticJan 23, 2021

Choose a reason for hiding this comment

Uh oh!

bstaleticJan 23, 2021

Choose a reason for hiding this comment

Uh oh!

bstaleticJan 23, 2021

Choose a reason for hiding this comment

Uh oh!

bstaleticJan 23, 2021

Choose a reason for hiding this comment

Uh oh!

bstaleticJan 23, 2021

Choose a reason for hiding this comment

Uh oh!

YannickJadoul left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YannickJadoulJan 23, 2021

Choose a reason for hiding this comment

Uh oh!

YannickJadoulJan 23, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

YannickJadoul commented Jan 24, 2021

Uh oh!

bstaletic commented Jul 14, 2021

Uh oh!

henryiii left a comment

Choose a reason for hiding this comment

Uh oh!

Skylion007 commented Jul 14, 2021

Uh oh!

bstaletic commented Jul 14, 2021

Uh oh!

rwgk left a comment

Choose a reason for hiding this comment

Uh oh!

rwgk commented Jul 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

YannickJadoul left a comment •
edited
Loading