[Bug libstdc++/37475] [DR 382] codecvt::do_in/do_out functions return "ok" when the output sequence has zero length

[Bug libstdc++/37475] [DR 382] codecvt::do_in/do_out functions return "ok" when the output sequence has zero length

[redi at gcc dot gnu.org]

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

--- Comment #3 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Issue 382 is NAD
http://cplusplus.github.io/LWG/lwg-defects.html#2369

[Bug libstdc++/37475] [DR 382] codecvt::do_in/do_out functions return "ok" when the output sequence has zero length

[kristian.spangsege at gmail dot com]

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

Kristian Spangsege <kristian.spangsege at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |kristian.spangsege at gmail dot co
                   |                            |m

--- Comment #5 from Kristian Spangsege <kristian.spangsege at gmail dot com> ---
This bug still exists in GCC 10.2, while libc++ (clang) appears to do the right
thing.

Specifically, when passing a nonzero amount of data (from_end > from) to
std::codecvt::out(), but an empty output buffer (to_end == to), the return
value is "ok". According to the C++ standard (C++17), the correct return value
is "partial".

This appears to happen regardless of locale. In particular, it happens in the C
locale.

As far as I can tell, http://cplusplus.github.io/LWG/lwg-closed.html#382 is not
relevant here, because the current standard text is clear (C++17), and
http://cplusplus.github.io/LWG/lwg-closed.html#382 has status NAD, which I
assume means "not a defect".

Jonathan, let me know if I am missing something.

[Bug libstdc++/37475] [DR 382] codecvt::do_in/do_out functions return "ok" when the output sequence has zero length

[kristian.spangsege at gmail dot com]

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

--- Comment #6 from Kristian Spangsege <kristian.spangsege at gmail dot com> ---
Created attachment 58405
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=58405&action=edit
Test whether bug still exists in GCC 14.0.1

[Bug libstdc++/37475] [DR 382] codecvt::do_in/do_out functions return "ok" when the output sequence has zero length

[kristian.spangsege at gmail dot com]

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

--- Comment #7 from Kristian Spangsege <kristian.spangsege at gmail dot com> ---
This bug still exists in GCC 14.0.1. This was tested with the attacked
test_2.cpp. The actual result is `ok`, but it should have been `partial`.

Curiously, this bug does not occur when using the Cygwin or MinGW versions of
GCC. In these cases, the result is `partial` as it should be. I assume this is
because on Cygwin and MinGW, libstdc++ uses a generic locale implementation
that is different from the one ordinarily used on Linux.

[Bug libstdc++/37475] [DR 382] codecvt::do_in/do_out functions return "ok" when the output sequence has zero length

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|SUSPENDED                   |NEW

--- Comment #8 from Jonathan Wakely <redi at gcc dot gnu.org> ---
I assume you mean 14.1.0

Since 382 indeed doesn't seem relevant here, let's unsuspend this.

[Bug libstdc++/37475] codecvt::do_in/do_out functions return "ok" when the output sequence has zero length

[redi at gcc dot gnu.org]

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

--- Comment #9 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Kristian Spangsege from comment #7)
> Curiously, this bug does not occur when using the Cygwin or MinGW versions
> of GCC. In these cases, the result is `partial` as it should be. I assume
> this is because on Cygwin and MinGW, libstdc++ uses a generic locale
> implementation that is different from the one ordinarily used on Linux.

It has an explicit check for this condition:

    // It is not clear that __from < __from_end implies __ret != ok
    // (see DR 382).
    if (__ret == ok && __from < __from_end)
      __ret = partial;

[Bug libstdc++/37475] codecvt::do_in/do_out functions return "ok" when the output sequence has zero length

[redi at gcc dot gnu.org]

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

--- Comment #10 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Whereas the GNU locale has that check inside a loop, which is never entered
when the destination buffer is zero sized, i.e. __to == __to_end

        if (__from_next < __from_end && __ret == ok)
          {
            if (__to_next < __to_end)
              {
                // XXX Probably wrong for stateful encodings
                __tmp_state = __state;
                ++__from_next;
                *__to_next++ = L'\0';
              }
            else
              __ret = partial;
          }

[Bug libstdc++/37475] codecvt::do_in/do_out functions return "ok" when the output sequence has zero length

[redi at gcc dot gnu.org]

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

--- Comment #11 from Jonathan Wakely <redi at gcc dot gnu.org> ---
This changes the loop to always run if the input is non-empty, and so return
partial if the destination is empty.

--- a/libstdc++-v3/config/locale/gnu/codecvt_members.cc
+++ b/libstdc++-v3/config/locale/gnu/codecvt_members.cc
@@ -131,10 +131,16 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
     // mbsnrtowcs is *very* fast but stops if encounters NUL characters:
     // in case we store a L'\0' and then continue, in a loop.
     // NB: mbsnrtowcs is a GNU extension
-    for (__from_next = __from, __to_next = __to;
-        __from_next < __from_end && __to_next < __to_end
-        && __ret == ok;)
+    __from_next = __from;
+    __to_next = __to;
+    while (__from_next < __from_end)
       {
+       if (__to_next >= __to_end)
+         {
+           __ret = partial;
+           break;
+         }
+
        const extern_type* __from_chunk_end;
        __from_chunk_end = static_cast<const extern_type*>(memchr(__from_next,
'\0',
                                                                  __from_end
@@ -162,12 +168,14 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
            __from_next = __from;
            __state = __tmp_state;
            __ret = error;
+           break;
          }
        else if (__from_next && __from_next < __from_chunk_end)
          {
            // It is unclear what to return in this case (see DR 382).
            __to_next += __conv;
            __ret = partial;
+           break;
          }
        else
          {
@@ -175,7 +183,7 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
            __to_next += __conv;
          }

-       if (__from_next < __from_end && __ret == ok)
+       if (__from_next < __from_end)
          {
            if (__to_next < __to_end)
              {
@@ -185,7 +193,10 @@ _GLIBCXX_BEGIN_NAMESPACE_VERSION
                *__to_next++ = L'\0';
              }
            else
-             __ret = partial;
+             {
+               __ret = partial;
+               break;
+             }
          }
       }

[Bug libstdc++/37475] codecvt::do_in/do_out functions return "ok" when the output sequence has zero length

[kristian.spangsege at gmail dot com]

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

--- Comment #12 from Kristian Spangsege <kristian.spangsege at gmail dot com> ---
These changes look good to me.

A couple of points:
* Should the `else`s still be there now that the preceding branches terminate
with `break`?
* `do_out()` has the same problem as `do_in()`.
* If there is already testing of `codecvt`, should a test be added for the
"empty output" case?
* This could break code that expects the incorrect behavior.

[Bug libstdc++/37475] codecvt::do_in/do_out functions return "ok" when the output sequence has zero length

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org
                URL|                            |https://gcc.gnu.org/piperma
                   |                            |il/gcc-patches/2024-June/65
                   |                            |4410.html
           Keywords|                            |patch

--- Comment #13 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Full patch: https://gcc.gnu.org/pipermail/gcc-patches/2024-June/654410.html

(In reply to Kristian Spangsege from comment #12)
> These changes look good to me.
> 
> A couple of points:
> * Should the `else`s still be there now that the preceding branches
> terminate with `break`?

Doesn't really matter either way.

> * `do_out()` has the same problem as `do_in()`.

Yes, addressed in the posted patch.

> * If there is already testing of `codecvt`, should a test be added for the
> "empty output" case?

Yes, addressed in the posted patch.

> * This could break code that expects the incorrect behavior.

That's true for every bug fix. We should conform to the standard though.

[Bug libstdc++/37475] codecvt::do_in/do_out functions return "ok" when the output sequence has zero length

[kristian.spangsege at gmail dot com]

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

--- Comment #14 from Kristian Spangsege <kristian.spangsege at gmail dot com> ---
Is the underscore intended before `Guard` in `explicit _Guard(__c_locale) { }`
for the GLIBC <= 2.2 case?

Besides that, the full patch looks correct to me.

[Bug libstdc++/37475] codecvt::do_in/do_out functions return "ok" when the output sequence has zero length

[redi at gcc dot gnu.org]

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

--- Comment #15 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Oops, that's definitely not intended! Good catch, thanks.

[Bug libstdc++/37475] codecvt::do_in/do_out functions return "ok" when the output sequence has zero length

[cvs-commit at gcc dot gnu.org]

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

--- Comment #16 from GCC Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:73ad57c244c283bf6da0c16630212f11b945eda5

commit r15-1693-g73ad57c244c283bf6da0c16630212f11b945eda5
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Tue Jun 11 16:45:43 2024 +0100

    libstdc++: Fix std::codecvt<wchar_t, char, mbstate_t> for empty dest
[PR37475]

    For the GNU locale model, codecvt::do_out and codecvt::do_in incorrectly
    return 'ok' when the destination range is empty. That happens because
    detecting incomplete output is done in the loop body, and the loop is
    never even entered if to == to_end.

    By restructuring the loop condition so that we check the output range
    separately, we can ensure that for a non-empty source range, we always
    enter the loop at least once, and detect if the destination range is too
    small.

    The loops also seem easier to reason about if we return immediately on
    any error, instead of checking the result twice on every iteration. We
    can use an RAII type to restore the locale before returning, which also
    simplifies all the other member functions.

    libstdc++-v3/ChangeLog:

            PR libstdc++/37475
            * config/locale/gnu/codecvt_members.cc (Guard): New RAII type.
            (do_out, do_in): Return partial if the destination is empty but
            the source is not. Use Guard to restore locale on scope exit.
            Return immediately on any conversion error.
            (do_encoding, do_max_length, do_length): Use Guard.
            * testsuite/22_locale/codecvt/in/char/37475.cc: New test.
            * testsuite/22_locale/codecvt/in/wchar_t/37475.cc: New test.
            * testsuite/22_locale/codecvt/out/char/37475.cc: New test.
            * testsuite/22_locale/codecvt/out/wchar_t/37475.cc: New test.

[Bug libstdc++/37475] codecvt::do_in/do_out functions return "ok" when the output sequence has zero length

[redi at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
   Target Milestone|---                         |15.0
         Resolution|---                         |FIXED

--- Comment #17 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Fixed on trunk (with the typo fixed).

[Bug libstdc++/37475] [DR 382] codecvt::do_in/do_out functions return "ok" when the output sequence has zero length

[paolo dot carlini at oracle dot com]

-- 

paolo dot carlini at oracle dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |SUSPENDED


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=37475

[Bug libstdc++/37475] [DR 382] codecvt::do_in/do_out functions return "ok" when the output sequence has zero length

[paolo dot carlini at oracle dot com]

-- 

paolo dot carlini at oracle dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
     Ever Confirmed|0                           |1
   Last reconfirmed|0000-00-00 00:00:00         |2008-09-11 17:39:18
               date|                            |
            Summary|codecvt::do_in/do_out       |[DR 382]
                   |functions return "ok" when  |codecvt::do_in/do_out
                   |the output sequence has zero|functions return "ok" when
                   |length                      |the output sequence has zero
                   |                            |length


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=37475