On Wed, 4 May 2022 at 12:14, Jonathan Wakely <jwakely....@gmail.com> wrote: > > On Tue, 3 May 2022 at 11:57, Jakob Hasse via Libstdc++ > <libstd...@gcc.gnu.org> wrote: > > > > This is a patch for the bug 105387 reported in bugzilla: 105387 > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=105387. This report should > > contain all the necessary information about the issue. But the patch there > > was just a preliminary one. > > I created a proper patch with the fix and a test case, based on TAG > > releases/gcc-11.2.0 (7ca388565af176bd4efd4f8db1e5e9e11e98ef45). The > > changelog is part of the commit message in the patch. > > Thanks for the patch! > > Some boring administrative comments: > > In the summary line of the git commit message: > - The component should be libstdc++ not libstdc++-v3. > - The PR number should include "PR" i.e. [PR105387]. > Your email Subject: gets this right, but the patch doesn't. > This is (not very well) documented at > https://gcc.gnu.org/contribute.html#patches > > Your new testcase has a FSF copyright notice. Unless you have already > completed the paperwork to assign your work (either for just this > change, or this and all future changes) to the FSF then you need to > either do that legal paperwork, or alternatively contribute under the > DCO terms without assigning copyright. See > https://gcc.gnu.org/contribute.html#legal > For simplicity and to expedite the process, I suggest just removing > the copyright notice and license notice from the testcase, and adding > a Signed-off-by: tag to the commit message, as per > https://gcc.gnu.org/dco.html > > As for the actual code ...
The commit msg says: "In case RTTI is enabled, this does not seem to be a problem because RTTI-based checks would run before and prevent running into the bad down-cast. However, since the check is very simple and I'm not 100% sure about the RTTI-case, it has been left for both cases (RTTI and no-RTTI)." I think we don't want to do the redundant __is_pointer_p() call for the RTTI-enabled case. For that case we know it's a pointer, or we'd have returned early, so making an extra virtual call is just unnecessary overhead. So I think your new check should be done in an #else branch of the existing #if __cpp_rtti block. More importantly, I think your patch breaks this case, which currently works with or without RTTI: struct X { int i; }; try { throw &X::i; } catch (const int X::*) { } For this case __is_pointer_p() is false, but the cast to __pbase_type_info is safe, and required for correctness. I think with your patch the exception won't be caught. Maybe that's a sacrifice we have to make in order to avoid undefined behaviour for your original example, but it's unfortunate if that's the case.