Re: [PATCH 4/4] dir.c: avoid "exceeds maximum object size" error with GCC v12.x

From: Junio C Hamano <gitster@pobox.com>
Date: May 25, 2022, 18:27 UTC
Message-ID: <xmqqzgj579ie.fsf@gitster.g>
In-Reply-To: <d7ee0bf0-22bc-d494-2699-7075fc92f28a@github.com>

Derrick Stolee <derrickstolee@github.com> writes:

Show 20 quoted lines

> On 5/24/2022 5:05 PM, Johannes Schindelin wrote:> On Tue, 24 May 2022, Ævar Arnfjörð Bjarmason wrote:
>>> On Tue, May 24 2022, Johannes Schindelin via GitGitGadget wrote:
>>>> +	/*
>>>> +	 * It should not be possible to overflow `ptrdiff_t` by passing in an
>>>> +	 * insanely long URL, but GCC does not know that and will complain
>>>> +	 * without this check.
>>>> +	 */
>>>> +	if (end - start < 0)
>>>> +		die(_("No directory name could be guessed.\n"
>>>
>>> This should start with a lower-case letter, see CodingGuidelines.
>> 
>> This message is copied from existing code later in the same function.
>> Since it is a translateable message, I do not want to edit it because that
>> would cause unnecessary work of the translators. Especially given that we
>> do not even expect this message to be shown, ever, but we only add this
>> hunk for GCC's benefit.
>
> Perhaps this should be a BUG() statement, then? Without any
> translation?

Yeah, both are good. If somehow the caller managed to pass such a long URL then it can be considered a data error at runtime, and not that the user detected a bug in our code, so in that sense die() would be appropriate. It is like xmalloc() running out of memory.

On the other hand, the "should not be possible to overflow" in the comment implicitly assumes that it is impossible to pass insanely long URL to trigger the condition from places we think of offhand, like the command line, where the input is limited to a much shorter string. As "we detected a situation that should not happen unless there is a programming or design bug" is what BUG() means, it is also good here---our assumption that this should not be possible turned out to be faulty, so we noticed a design bug.

I wonder if we can add a separate macro to add more to the documentation value, though. With something like

    #define FALSE_WARNING(expression, message) \
	do { if (expression) { BUG(message); } while (0)

the above would just become

	FALSE_WARNING(end - start < 0, "ptrdiff_t would not overflow here");

without a need for a big comment before it. We might even be able to optimize it out when building with compilers that do not need the workaround.