From: Daniel Gustafsson <daniel@yesql.se>
Message-Id: <0A596D8B-24F3-408F-B89B-B42014B00A05@yesql.se>
Content-Type: multipart/mixed;
	boundary="Apple-Mail=_BBA65E04-AB4E-47D4-BDD7-4F707143100C"
Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3776.700.51.11.1\))
Subject: Re: jsonapi: scary new warnings with LTO enabled
Date: Thu, 17 Apr 2025 13:43:18 +0200
In-Reply-To: <2087556.1744846124@sss.pgh.pa.us>
Cc: Jacob Champion <jacob.champion@enterprisedb.com>,
 pgsql-hackers@lists.postgresql.org
To: Tom Lane <tgl@sss.pgh.pa.us>
References: <2074634.1744839761@sss.pgh.pa.us>
 <0A3CF5C4-9947-42E3-89A9-34FCF7BA95BA@yesql.se>
 <2078511.1744841564@sss.pgh.pa.us>
 <DCD56158-A642-4954-87FF-9DAD5425F3FF@yesql.se>
 <2084687.1744844690@sss.pgh.pa.us>
 <CAOYmi+n+DHpaEV5hymhufutTgc+S=+pDQfXEUi4hMAV1eO8aTw@mail.gmail.com>
 <2087556.1744846124@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/0A596D8B-24F3-408F-B89B-B42014B00A05%40yesql.se>
Precedence: bulk


--Apple-Mail=_BBA65E04-AB4E-47D4-BDD7-4F707143100C
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain;
	charset=utf-8

> On 17 Apr 2025, at 01:28, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>=20
> Jacob Champion <jacob.champion@enterprisedb.com> writes:
>> On Wed, Apr 16, 2025 at 4:04=E2=80=AFPM Tom Lane <tgl@sss.pgh.pa.us> =
wrote:
>>> Looking through all of the callers of freeJsonLexContext, quite
>>> a lot of them use local JsonLexContext structs, and probably some
>>> of them are more performance-critical than these.  So that raises
>>> the question of why are we seeing warnings for only these call
>>> sites?
>=20
>> Yeah, I had the same question...
>=20
> After making another pass through the callers of freeJsonLexContext,
> I observe that the warnings appear in callers that use a local
> variable *and* contain goto statements.  So I'm betting that the
> presence of goto's causes the LTO optimizer to pull in its horns
> quite a bit and thereby fail to detect the flag correlation.

That seems plausible given the selective warnings.

>>> Maybe there is a more elegant way to suppress them.
>=20
>> Can we brute-force ignore this particular warning site with a #pragma
>> (suggested in [1])?
>=20
> That's surely not elegant :-(.  However, I don't especially want to
> rewrite away the goto's in these callers ...

Agreed, moving to heap allocated structures for these callsites seem =
much
better. Something like the attached should be enough I think?

--
Daniel Gustafsson


--Apple-Mail=_BBA65E04-AB4E-47D4-BDD7-4F707143100C
Content-Disposition: attachment;
	filename=0001-Allocate-JsonLexContexts-on-the-heap-to-avoid-warnin.patch
Content-Type: application/octet-stream;
	x-unix-mode=0644;
	name="0001-Allocate-JsonLexContexts-on-the-heap-to-avoid-warnin.patch"
Content-Transfer-Encoding: quoted-printable

=46rom=20c5b661e24aff9d6ae54f922a289b92d39383322d=20Mon=20Sep=2017=20=
00:00:00=202001=0AFrom:=20Daniel=20Gustafsson=20=
<dgustafsson@postgresql.org>=0ADate:=20Thu,=2017=20Apr=202025=2013:38:05=20=
+0200=0ASubject:=20[PATCH]=20Allocate=20JsonLexContexts=20on=20the=20=
heap=20to=20avoid=20warnings=0A=0AThe=20stack=20allocated=20=
JsonLexContexts,=20in=20combination=20with=20codepaths=0Ausing=20goto,=20=
were=20causing=20warnings=20when=20compiling=20with=20LTO=20enabled=0Aas=20=
the=20optimizer=20is=20unable=20to=20figure=20out=20that=20is=20safe.=20=20=
Rather=20than=0Acontort=20the=20code=20with=20workarounds=20for=20this=20=
simply=20heap=20allocate=20the=0Astructs=20instead=20as=20these=20are=20=
not=20in=20any=20performance=20critical=20paths.=0A=0AReported-by:=20Tom=20=
Lane=20<tgl@sss.pgh.pa.us>=0ADiscussion:=20=
https://postgr.es/m/2074634.1744839761@sss.pgh.pa.us=0A---=0A=20=
src/interfaces/libpq/fe-auth-oauth.c=20=20=20=20=20=20=20=20=20=20|=2023=20=
++++++++++++++-----=0A=20.../test_json_parser_incremental.c=20=20=20=20=20=
=20=20=20=20=20=20=20|=2022=20++++++++++--------=0A=202=20files=20=
changed,=2030=20insertions(+),=2015=20deletions(-)=0A=0Adiff=20--git=20=
a/src/interfaces/libpq/fe-auth-oauth.c=20=
b/src/interfaces/libpq/fe-auth-oauth.c=0Aindex=20=
cf1a25e2ccc..b4d69874c83=20100644=0A---=20=
a/src/interfaces/libpq/fe-auth-oauth.c=0A+++=20=
b/src/interfaces/libpq/fe-auth-oauth.c=0A@@=20-476,7=20+476,7=20@@=20=
issuer_from_well_known_uri(PGconn=20*conn,=20const=20char=20*wkuri)=0A=20=
static=20bool=0A=20handle_oauth_sasl_error(PGconn=20*conn,=20const=20=
char=20*msg,=20int=20msglen)=0A=20{=0A-=09JsonLexContext=20lex=20=3D=20=
{0};=0A+=09JsonLexContext=20*lex;=0A=20=09JsonSemAction=20sem=20=3D=20=
{0};=0A=20=09JsonParseErrorType=20err;=0A=20=09struct=20json_ctx=20ctx=20=
=3D=20{0};=0A@@=20-504,8=20+504,19=20@@=20handle_oauth_sasl_error(PGconn=20=
*conn,=20const=20char=20*msg,=20int=20msglen)=0A=20=09=09return=20false;=0A=
=20=09}=0A=20=0A-=09makeJsonLexContextCstringLen(&lex,=20msg,=20msglen,=20=
PG_UTF8,=20true);=0A-=09setJsonLexContextOwnsTokens(&lex,=20true);=09/*=20=
must=20not=20leak=20on=20error=20*/=0A+=09/*=0A+=09=20*=20Later=20error=20=
paths=20need=20to=20go=20via=20the=20cleanup=20label=20but=20since=20ctx=20=
hasn't=0A+=09=20*=20been=20initiated=20yet=20we=20return=20immediately=20=
here.=0A+=09=20*/=0A+=09lex=20=3D=20calloc(1,=20sizeof(JsonLexContext));=0A=
+=09if=20(!lex)=0A+=09{=0A+=09=09libpq_append_conn_error(conn,=20"out=20=
of=20memory");=0A+=09=09return=20false;=0A+=09}=0A+=0A+=09=
makeJsonLexContextCstringLen(lex,=20msg,=20msglen,=20PG_UTF8,=20true);=0A=
+=09setJsonLexContextOwnsTokens(lex,=20true);=09/*=20must=20not=20leak=20=
on=20error=20*/=0A=20=0A=20=09initPQExpBuffer(&ctx.errbuf);=0A=20=09=
sem.semstate=20=3D=20&ctx;=0A@@=20-516,7=20+527,7=20@@=20=
handle_oauth_sasl_error(PGconn=20*conn,=20const=20char=20*msg,=20int=20=
msglen)=0A=20=09sem.array_start=20=3D=20oauth_json_array_start;=0A=20=09=
sem.scalar=20=3D=20oauth_json_scalar;=0A=20=0A-=09err=20=3D=20=
pg_parse_json(&lex,=20&sem);=0A+=09err=20=3D=20pg_parse_json(lex,=20=
&sem);=0A=20=0A=20=09if=20(err=20=3D=3D=20JSON_SEM_ACTION_FAILED)=0A=20=09=
{=0A@@=20-535,7=20+546,7=20@@=20handle_oauth_sasl_error(PGconn=20*conn,=20=
const=20char=20*msg,=20int=20msglen)=0A=20=09=09}=0A=20=09}=0A=20=09else=20=
if=20(err=20!=3D=20JSON_SUCCESS)=0A-=09=09errmsg=20=3D=20=
json_errdetail(err,=20&lex);=0A+=09=09errmsg=20=3D=20json_errdetail(err,=20=
lex);=0A=20=0A=20=09if=20(errmsg)=0A=20=09=09=
libpq_append_conn_error(conn,=0A@@=20-544,7=20+555,7=20@@=20=
handle_oauth_sasl_error(PGconn=20*conn,=20const=20char=20*msg,=20int=20=
msglen)=0A=20=0A=20=09/*=20Don't=20need=20the=20error=20buffer=20or=20=
the=20JSON=20lexer=20anymore.=20*/=0A=20=09termPQExpBuffer(&ctx.errbuf);=0A=
-=09freeJsonLexContext(&lex);=0A+=09freeJsonLexContext(lex);=0A=20=0A=20=09=
if=20(errmsg)=0A=20=09=09goto=20cleanup;=0Adiff=20--git=20=
a/src/test/modules/test_json_parser/test_json_parser_incremental.c=20=
b/src/test/modules/test_json_parser/test_json_parser_incremental.c=0A=
index=20a529ee47e9b..357152e0ae0=20100644=0A---=20=
a/src/test/modules/test_json_parser/test_json_parser_incremental.c=0A+++=20=
b/src/test/modules/test_json_parser/test_json_parser_incremental.c=0A@@=20=
-84,7=20+84,7=20@@=20main(int=20argc,=20char=20**argv)=0A=20=09char=09=09=
buff[BUFSIZE];=0A=20=09FILE=09=20=20=20*json_file;=0A=20=09=
JsonParseErrorType=20result;=0A-=09JsonLexContext=20lex;=0A+=09=
JsonLexContext=20*lex;=0A=20=09StringInfoData=20json;=0A=20=09int=09=09=09=
n_read;=0A=20=09size_t=09=09chunk_size=20=3D=20DEFAULT_CHUNK_SIZE;=0A@@=20=
-98,6=20+98,10=20@@=20main(int=20argc,=20char=20**argv)=0A=20=0A=20=09=
pg_logging_init(argv[0]);=0A=20=0A+=09lex=20=3D=20calloc(1,=20=
sizeof(JsonLexContext));=0A+=09if=20(!lex)=0A+=09=09pg_fatal("out=20of=20=
memory");=0A+=0A=20=09while=20((c=20=3D=20getopt(argc,=20argv,=20=
"c:os"))=20!=3D=20-1)=0A=20=09{=0A=20=09=09switch=20(c)=0A@@=20-113,7=20=
+117,7=20@@=20main(int=20argc,=20char=20**argv)=0A=20=09=09=09case=20=
's':=09=09=09/*=20do=20semantic=20processing=20*/=0A=20=09=09=09=09=
testsem=20=3D=20&sem;=0A=20=09=09=09=09sem.semstate=20=3D=20=
palloc(sizeof(struct=20DoState));=0A-=09=09=09=09((struct=20DoState=20*)=20=
sem.semstate)->lex=20=3D=20&lex;=0A+=09=09=09=09((struct=20DoState=20*)=20=
sem.semstate)->lex=20=3D=20lex;=0A=20=09=09=09=09((struct=20DoState=20*)=20=
sem.semstate)->buf=20=3D=20makeStringInfo();=0A=20=09=09=09=09=
need_strings=20=3D=20true;=0A=20=09=09=09=09break;=0A@@=20-131,8=20=
+135,8=20@@=20main(int=20argc,=20char=20**argv)=0A=20=09=09exit(1);=0A=20=
=09}=0A=20=0A-=09makeJsonLexContextIncremental(&lex,=20PG_UTF8,=20=
need_strings);=0A-=09setJsonLexContextOwnsTokens(&lex,=20=
lex_owns_tokens);=0A+=09makeJsonLexContextIncremental(lex,=20PG_UTF8,=20=
need_strings);=0A+=09setJsonLexContextOwnsTokens(lex,=20=
lex_owns_tokens);=0A=20=09initStringInfo(&json);=0A=20=0A=20=09if=20=
((json_file=20=3D=20fopen(testfile,=20PG_BINARY_R))=20=3D=3D=20NULL)=0A=
@@=20-165,12=20+169,12=20@@=20main(int=20argc,=20char=20**argv)=0A=20=09=09=
bytes_left=20-=3D=20n_read;=0A=20=09=09if=20(bytes_left=20>=200)=0A=20=09=
=09{=0A-=09=09=09result=20=3D=20pg_parse_json_incremental(&lex,=20=
testsem,=0A+=09=09=09result=20=3D=20pg_parse_json_incremental(lex,=20=
testsem,=0A=20=09=09=09=09=09=09=09=09=09=09=09=20=20=20json.data,=20=
n_read,=0A=20=09=09=09=09=09=09=09=09=09=09=09=20=20=20false);=0A=20=09=09=
=09if=20(result=20!=3D=20JSON_INCOMPLETE)=0A=20=09=09=09{=0A-=09=09=09=09=
fprintf(stderr,=20"%s\n",=20json_errdetail(result,=20&lex));=0A+=09=09=09=
=09fprintf(stderr,=20"%s\n",=20json_errdetail(result,=20lex));=0A=20=09=09=
=09=09ret=20=3D=201;=0A=20=09=09=09=09goto=20cleanup;=0A=20=09=09=09}=0A=
@@=20-178,12=20+182,12=20@@=20main(int=20argc,=20char=20**argv)=0A=20=09=09=
}=0A=20=09=09else=0A=20=09=09{=0A-=09=09=09result=20=3D=20=
pg_parse_json_incremental(&lex,=20testsem,=0A+=09=09=09result=20=3D=20=
pg_parse_json_incremental(lex,=20testsem,=0A=20=09=09=09=09=09=09=09=09=09=
=09=09=20=20=20json.data,=20n_read,=0A=20=09=09=09=09=09=09=09=09=09=09=09=
=20=20=20true);=0A=20=09=09=09if=20(result=20!=3D=20JSON_SUCCESS)=0A=20=09=
=09=09{=0A-=09=09=09=09fprintf(stderr,=20"%s\n",=20=
json_errdetail(result,=20&lex));=0A+=09=09=09=09fprintf(stderr,=20=
"%s\n",=20json_errdetail(result,=20lex));=0A=20=09=09=09=09ret=20=3D=20=
1;=0A=20=09=09=09=09goto=20cleanup;=0A=20=09=09=09}=0A@@=20-195,7=20=
+199,7=20@@=20main(int=20argc,=20char=20**argv)=0A=20=0A=20cleanup:=0A=20=
=09fclose(json_file);=0A-=09freeJsonLexContext(&lex);=0A+=09=
freeJsonLexContext(lex);=0A=20=09free(json.data);=0A=20=0A=20=09return=20=
ret;=0A--=20=0A2.39.3=20(Apple=20Git-146)=0A=0A=

--Apple-Mail=_BBA65E04-AB4E-47D4-BDD7-4F707143100C--