Re: [Qemu-devel] [PATCH v3 01/12] qobject: Accept "%"PRId64 in qobject_from_jsonf()

[Markus Armbruster <armbru@redhat.com>]

Eric Blake <eblake@redhat.com> writes:

> Commit 1792d7d0 was written because PRId64 expands to non-portable
> crap for some libc, and we had testsuite failures on Mac OS as a
> result.  This in turn makes it difficult to rely on the obvious
> conversions of 64-bit values into JSON, requiring things such as
> casting int64_t to long long so we can use a reliable %lld and
> still keep -Wformat happy.  So now it's time to fix that.
>
> We are already lexing %I64d (hello mingw); now expand the lexer
> to parse %qd (hello Mac OS). In the process, we can drop one
> state: IN_ESCAPE_I64 is redundant with IN_ESCAPE_LL, and reused
> again by %qd, so rename it to IN_ESCAPE_64.  And fix a comment
> that mistakenly omitted %u as a supported escape.
>
> Next, tweak the parser to accept the exact spelling of PRId64
> regardless of what it expands to (note that there are are now dead
> branches in the 'if' chain for some platforms, like glibc; but
> there are other platforms where all branches are necessary).  We
> are at least safe in that we are parsing the correct size 32-bit or
> a 64-bit quantity on whatever branch we end up in.  This also means
> that we no longer parse everything that the lexer will consume (no
> more %I64d on glibc), but that is intentional.  And of course,
> update the testsuite for coverage of the feature.
>
> Finally, update configure to barf on any libc that uses yet
> another form of PRId64 that we have not yet coded for, so that we
> can once again update json-lexer.c to cater to those quirks (my
> guess? we might see %jd next) (on the bright side, json-parser.c
> should no longer need updates).  Yes, the only way I could find
> to quickly learn what spelling is preferred when cross-compiling
> is to grep a compiled binary; C does not make it easy to turn a
> string constant into an integer constant, let alone make
> preprocessor decisions; and even parsing '$CC -E' output is
> fragile, since 64-bit glibc pre-processes as "l" "d" rather than
> "ld", and newer gcc splits macro expansions across multiple lines.
> I'm assuming that 'strings' is portable enough during configure;
> if that assumption fails in some constrained docker environment,
> an easy hack is to add this to configure:
>   strings () { tr -d '\0' < "$1"; }
>
> Signed-off-by: Eric Blake <eblake@redhat.com>
>
> ---
> v3: incorporate review comments from Markus
> ---
>  configure             | 24 ++++++++++++++++++++++++
>  qobject/json-lexer.c  | 21 +++++++++------------
>  qobject/json-parser.c | 10 ++++++----
>  tests/check-qjson.c   |  7 +++++++
>  4 files changed, 46 insertions(+), 16 deletions(-)
>
> diff --git a/configure b/configure
> index 987f59ba88..5b0eddec3c 100755
> --- a/configure
> +++ b/configure
> @@ -3222,6 +3222,30 @@ for i in $glib_modules; do
>      fi
>  done
>
> +# Sanity check that we can parse PRId64 and friends in json-lexer.c
> +# (Sadly, the "easiest" way to do this is to grep the compiled binary,
> +# since we can't control whether the preprocessor would output "ld" vs.
> +# "l" "d", nor even portably ensure it fits output on the same line as
> +# a witness marker.)
> +cat > $TMPC <<EOF
> +#include <inttypes.h>
> +int main(void) {
> +    static const char findme[] = "\nUnLiKeLyToClAsH_" PRId64 "\n";
> +    return !findme[0];
> +}
> +EOF
> +if ! compile_prog "$CFLAGS" "$LIBS" ; then

Would compile_object suffice?

> +    error_exit "can't determine value of PRId64"
> +fi
> +nl='
> +'
> +case $(strings $TMPE | grep ^UnLiKeLyToClAsH) in
> +    '' | *"$nl"* ) error_exit "can't determine value of PRId64" ;;
> +    *_ld | *_lld | *_I64d | *_qd) ;;
> +    *) error_exit "unexepected value of PRId64, please add %$(strings $TMPE |
> +       sed -n s/^UnLiKeLyToClAsH_//p) support to json-lexer.c" ;;
> +esac
> +
>  # Sanity check that the current size_t matches the
>  # size that glib thinks it should be. This catches
>  # problems on multi-arch where people try to build
> diff --git a/qobject/json-lexer.c b/qobject/json-lexer.c
> index 980ba159d6..9a0fc58444 100644
> --- a/qobject/json-lexer.c
> +++ b/qobject/json-lexer.c
> @@ -29,9 +29,11 @@
>   *
>   * '([^\\']|\\[\"'\\/bfnrt]|\\u[0-9a-fA-F]{4})*'
>   *
> - * Extension for vararg handling in JSON construction:
> + * Extension for vararg handling in JSON construction [this lexer
> + * actually accepts multiple forms of PRId64, but parse_escape() later
> + * filters to only parse the current platform's spelling]:

I'd stick to (parenthesis) instead of [square brackets] here.

>   *
> - * %((l|ll|I64)?d|[ipsf])
> + * %(PRI[du]64|(l|ll)?[ud]|[ipsf])
>   *
>   */
>
[...]

Reviewed-by: Markus Armbruster <armbru@redhat.com>