public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
From: Florian Weimer <fweimer@redhat.com>
To: Zack Weinberg <zackw@panix.com>,
	libc-alpha@sourceware.org,
	"Gabriel F. T. Gomes" <gftg@linux.vnet.ibm.com>
Subject: Re: [PATCH 0/9] Use more flags parameters instead of global bits in stdio
Date: Wed, 27 Jun 2018 15:50:00 -0000	[thread overview]
Message-ID: <02d8eb49-00da-196a-66b2-66e376b3f66d@redhat.com> (raw)
In-Reply-To: <20180307193205.4751-1-zackw@panix.com>

On 03/07/2018 08:31 PM, Zack Weinberg wrote:
> I got stuck on the patch to use C99-compliant scanf in _GNU_SOURCE
> mode because the interaction with ldbl-is-dbl was too confusing.  The
> reason it's too confusing is that C99 compliance in scanf, ldbl-is-dbl
> mode in scanf, printf, and strfmon, and fortify mode in printf are
> handled with mode bits on the FILE and thread-global flags that must
> be set and reset at just the right times.  Correct behavior is
> invariably to set and then reset around just one call to a lower-level
> function, and there's a better way to do that: flags parameters.

I looked at how this change interacts with printf format specifier 
callbacks.

There currently does not appear to be a way to determine in the callback 
if an L argument was of double or long double type.  There is code to 
adjust the argument type for double mode:

       case PA_DOUBLE|PA_FLAG_LONG_DOUBLE:
         if (__ldbl_is_dbl)
           {
             args_value[cnt].pa_double = va_arg (*ap_savep, double);
             args_type[cnt] &= ~PA_FLAG_LONG_DOUBLE;
           }
         else
           args_value[cnt].pa_long_double = va_arg (*ap_savep, long double);
         break;

But I don't think args_type is ever read back, and it's not really 
accessible to the second callback function afterwards.

With the thread-local variable, you can run something like this to 
determine if you are in double double or binary64 mode because snprintf 
will not reset the __no_long_double internal TLS variable:

static bool
is_long_double_mode (void)
{
   char buf[64];
   extern __typeof__ (snprintf) snprintf_alias __asm__ ("snprintf");
   snprintf_alias (buf, sizeof (buf), "%.30Lf",
                   1234.0000000000000000000001L);
   puts (buf);
   return strcmp (buf, "1234.000000000000000000000099999997") == 0;
}

There does not seem to be any other way to get at this variable, so I'm 
not sure this is something we need to support going forward.  The flag 
is not copied into the FILE * struct, either.  Considering that 
is_long_double_mode is so inefficient, I don't think this is anything to 
worry about for real code.

For the _IO_FLAGS2_FORTIFY flag, things are a bit different.  It is 
currently copied into the FILE * struct, so it is in theory accessible 
to the printf callbacks.  But it's now in an internal header, and it 
seems unlikely that any code would use it given that it was 
underdocumented before.  Again, this doesn't look a practical problem.

This concern does not apply to _IO_FLAGS2_SCANF_STD because there are no 
scanf hooks, so there isn't any problem there.

So I think this means that the change from thread-local variable and 
in-FILE flags to an argument is conceptually valid.  I have only started 
to review the implementation, though.

Thanks,
Florian

      parent reply	other threads:[~2018-06-27 15:50 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-07 19:32 Zack Weinberg
2018-03-07 19:32 ` [PATCH 3/9] Use SCANF_ISOC99_A instead of _IO_FLAGS2_SCANF_STD Zack Weinberg
2018-03-26 15:35   ` Gabriel F. T. Gomes
2018-03-07 19:32 ` [PATCH 8/9] Use PRINTF_LDBL_IS_DBL instead of __ldbl_is_dbl Zack Weinberg
2018-03-07 19:32 ` [PATCH 4/9] Use SCANF_LDBL_IS_DBL " Zack Weinberg
2018-03-14 12:22   ` Florian Weimer
2018-03-26 15:36   ` Gabriel F. T. Gomes
2018-03-07 19:32 ` [PATCH 2/9] Add __vfscanf_internal and __vfwscanf_internal with flags arguments Zack Weinberg
2018-03-13 12:35   ` Adhemerval Zanella
2018-06-29 14:04     ` Florian Weimer
2018-03-26 15:28   ` Gabriel F. T. Gomes
2018-06-29 14:12     ` Florian Weimer
2018-06-29 14:24   ` Florian Weimer
2018-06-29 14:29   ` Florian Weimer
2018-03-07 19:32 ` [PATCH 6/9] Add __vsyslog_internal, with same flags as __v*printf_internal Zack Weinberg
2018-03-13 11:59   ` Florian Weimer
2018-03-13 12:39     ` Zack Weinberg
2018-03-13 12:43       ` Florian Weimer
2018-03-13 13:37         ` Zack Weinberg
2018-03-13 13:50           ` Florian Weimer
2018-03-13 14:11             ` Zack Weinberg
2018-03-13 14:13               ` Florian Weimer
2018-03-07 19:32 ` [PATCH 5/9] Add __v*printf_internal with flags arguments Zack Weinberg
2018-03-26 15:41   ` Gabriel F. T. Gomes
2018-03-07 19:32 ` [PATCH 1/9] Use STRFMON_LDBL_IS_DBL instead of __ldbl_is_dbl Zack Weinberg
2018-03-12 20:36   ` Adhemerval Zanella
2018-03-12 21:11     ` Zack Weinberg
2018-03-13 11:45       ` Adhemerval Zanella
2018-03-26 15:17   ` Gabriel F. T. Gomes
2018-03-26 15:40     ` Zack Weinberg
2018-03-26 15:52       ` Gabriel F. T. Gomes
2018-03-07 19:51 ` [PATCH 7/9] Use PRINTF_FORTIFY instead of _IO_FLAGS2_FORTIFY Zack Weinberg
2018-03-07 19:51 ` [PATCH 9/9] Post-cleanup: don't include math.h/math_private.h in math_ldbl_opt.h Zack Weinberg
2018-03-12 15:29 ` [PATCH 0/9] Use more flags parameters instead of global bits in stdio Zack Weinberg
2018-03-26 15:16 ` Gabriel F. T. Gomes
2018-03-26 15:47   ` Zack Weinberg
2018-06-27 15:50 ` Florian Weimer [this message]

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=02d8eb49-00da-196a-66b2-66e376b3f66d@redhat.com \
    --to=fweimer@redhat.com \
    --cc=gftg@linux.vnet.ibm.com \
    --cc=libc-alpha@sourceware.org \
    --cc=zackw@panix.com \
    /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).