public inbox for gcc-bugs@sourceware.org
help / color / mirror / Atom feed
From: "redi at gcc dot gnu.org" <gcc-bugzilla@gcc.gnu.org>
To: gcc-bugs@gcc.gnu.org
Subject: [Bug libstdc++/113099] locale without RTTI uses dynamic_cast before gcc 13.2 or has ODR violation since gcc 13.2
Date: Wed, 03 Jan 2024 12:38:14 +0000	[thread overview]
Message-ID: <bug-113099-4-HnGHdiVga6@http.gcc.gnu.org/bugzilla/> (raw)
In-Reply-To: <bug-113099-4@http.gcc.gnu.org/bugzilla/>

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113099

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |FIXED
             Status|UNCONFIRMED                 |RESOLVED
   Target Milestone|---                         |13.2

--- Comment #14 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to andysem from comment #13)
> (In reply to Jonathan Wakely from comment #12)
> > (In reply to andysem from comment #11)
> > > > I'm not sure what you mean by "the compiler is free to generate code that takes it into account." Takes what into account? What problem does that freedom cause?
> > > 
> > > I mean the compiler could move (some part of) dynamic_cast to precede the
> > > check for the standard facet. I.e. of something like this:
> > > 
> > >   template< typename _Facet >
> > >   const _Facet* __try_use_facet(locale const& loc)
> > >   {
> > >     const size_t __i = _Facet::id._M_id();
> > >     const locale::facet** __facets = __loc._M_impl->_M_facets;
> > >     const _Facet* __dyn_facet = __dynamic_cast< const _Facet*
> > > >(__facets[__i]);
> > > 
> > >     // checks for every standard facet type
> > >     if (__is_same(_Facet, ...))
> > >       return static_cast<const _Facet*>(__facets[__i]);
> > > 
> > >     return __dyn_facet;
> > >   }
> > 
> > But why? Maybe I'm being slow but I still don't understand what problem is
> > being solved here.
> > 
> > Also this would defeat the optimization that avoids the unnecessary overhead
> > of dynamic_cast for standard facets.
> 
> I have seen gcc sometimes reorder code like this (i.e. move code from under
> a branch before it), presumably to improve ILP or prefetch data, I'm not
> sure. Yes, that defeats the branch, if it is used as an optimization, and I
> had to prevent this by adding compiler fences in those cases. Granted, in my
> case it happened with inlined code, but I imagine LTO makes it possible to
> perform similar code transformations with out-of-line code as well.


Building libstdc++ with LTO is unsupported (and doesn't happen when building
GCC, it requires making custom changes to the build process, which again is
unsupported). That's a non-issue.

> I'm not saying this is what actually happens. I'm just pointing out that
> even the code that is apparently unreachable may influence codegen and cause
> ODR issues.

I'm still not seeing an actual problem here, just speculation.

The compiler knows that `if (__is_same(T,T))` is true, and will not move
unreachable code before it unless that code has no side effects. And if it has
no side effects, there's no problem.

Since GCC 13.2 there is no change in semantics or observable side effects when
the explicit instantiations for std::use_facet (which are compiled with -frtti)
are used, or the function is inlined (which might be compiled with -fno-rtti).

For the gcc-11 and gcc-12 branches would could suppress the explicit
instantiation declarations for -fno-rtti, so that the extern template
definitions do not try to use dynamic_cast on a type that has no RTTI. But then
we'd need to backport (at least part of) the PR 103755 changes to use
static_cast for standard facets. Otherwise std::use_facet and std::has_facet
would always fail, even for the standard facets which are guaranteed to be
present. I don't think we want to do that on the stable branches, so I think
this is WONTFIX for GCC 13.1 and earlier, and FIXED for 13.2 and later.

  parent reply	other threads:[~2024-01-03 12:38 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-12-20 23:41 [Bug libstdc++/113099] New: " andysem at mail dot ru
2023-12-21  8:59 ` [Bug libstdc++/113099] " rguenth at gcc dot gnu.org
2023-12-21 21:49 ` redi at gcc dot gnu.org
2023-12-23  9:37 ` andysem at mail dot ru
2023-12-23  9:39 ` andysem at mail dot ru
2023-12-24 14:44 ` redi at gcc dot gnu.org
2023-12-24 14:46 ` redi at gcc dot gnu.org
2023-12-24 15:04 ` pdimov at gmail dot com
2023-12-24 15:27 ` redi at gcc dot gnu.org
2023-12-24 15:30 ` redi at gcc dot gnu.org
2023-12-24 15:40 ` pdimov at gmail dot com
2023-12-24 16:22 ` andysem at mail dot ru
2023-12-26 12:31 ` redi at gcc dot gnu.org
2023-12-26 15:20 ` andysem at mail dot ru
2024-01-03 12:38 ` redi at gcc dot gnu.org [this message]
2024-01-03 12:43 ` redi at gcc dot gnu.org
2024-01-05 10:23 ` cvs-commit at gcc dot gnu.org

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=bug-113099-4-HnGHdiVga6@http.gcc.gnu.org/bugzilla/ \
    --to=gcc-bugzilla@gcc.gnu.org \
    --cc=gcc-bugs@gcc.gnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).