Subject: Re: jsonapi: scary new warnings with LTO enabled

From: Tom Lane <[email-masked]>
Date: Wed, 16 Apr 2025 18:12:44 -0400

Daniel Gustafsson <[email-masked]> writes:
>> On 16 Apr 2025, at 23:42, Tom Lane <[email-masked]> wrote:
>> I'm not sure
>> how other than giving up on stack allocation of JsonLexContexts,
>> though, especially if we consider the jsonapi API frozen.  But seeing
>> that there are only three such call sites and none of them seem in the
>> least performance-critical, maybe we should just do that?

> I can't see any other option really, and there is no performance angle really
> so that should be safe.  Since I committed at least one of these, let me know
> if you want me to tackle it.

The only alternative I can see that might stop the warning is if we
can find a way to make it clearer to the optimizer that the FREE()
isn't reached.  But I'm not sure about a trustworthy way to make that
happen.  Maybe it'd work to change the signature of freeJsonLexContext
(or perhaps better, add a separate entry point) so that the caller is
passing a bool constant that controls whether to free the struct.
We could have an Assert that compares that to the state of the
JSONLEX_FREE_STRUCT flag to catch mistakes.  This seems kind of messy
though.

			regards, tom lane

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 17 Apr 2025 00:18:23 +0200

> On 17 Apr 2025, at 00:12, Tom Lane <[email-masked]> wrote:
> 

Yeah, that seems messy enough that someone down the line will go "why on earth"
and we'll have to revisit this discussion.  It can probably be made to work but
I doubt it will be worth it compared to allocating on the heap.

--
Daniel Gustafsson

---

From: Tom Lane <[email-masked]>
Date: Wed, 16 Apr 2025 19:04:50 -0400

Daniel Gustafsson <[email-masked]> writes:
>> happen.

> I doubt it will be worth it compared to allocating on the heap.

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?  Maybe there is a more elegant way to suppress them.

Still, I think that observation refutes my initial thought that
we should rip out support for local JsonLexContext structs
altogether.  I'm inclined now to just do the minimal thing of
changing these callers to use an allocated struct, and call it
a day.  (BTW, there seem to be only 2 places to change not 3;
two of the warnings are pointing at the same variable.)

			regards, tom lane

---

From: Jacob Champion <[email-masked]>
Date: Wed, 16 Apr 2025 16:09:40 -0700

On Wed, Apr 16, 2025 at 4:04 PM Tom Lane <[email-masked]> wrote:
> sites?

Yeah, I had the same question...

> Maybe there is a more elegant way to suppress them.

Can we brute-force ignore this particular warning site with a #pragma
(suggested in [1])?

--Jacob

[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=98753

---

From: Tom Lane <[email-masked]>
Date: Wed, 16 Apr 2025 19:28:44 -0400

> Yeah, I had the same question...

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.

> Can we brute-force ignore this particular warning site with a #pragma

That's surely not elegant :-(.  However, I don't especially want to
rewrite away the goto's in these callers ...

			regards, tom lane

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 17 Apr 2025 13:43:18 +0200

> On 17 Apr 2025, at 01:28, Tom Lane <[email-masked]> wrote:
> 

That seems plausible given the selective warnings.

> 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