#4003 Wrong diagnostic message when adding 0 to a pointer to an _Optional type

[Christopher Bazley]

1: Sample code that reproduces the problem.

int *black(_Optional int *poi)
{
  return 0 + poi; // recommended diagnostic
}

2: Exact command used to run SDCC on this sample code

sdcc --stack-auto -c black.c

3: SDCC version tested (type "sdcc -v" to find it)

SDCC : mcs51/z80/z180/r2k/r2ka/r3ka/r4k/r5k/r6k/sm83/tlcs90/ez80/z80n/r800/ds390/pic16/pic14/TININative/ds400/hc08/s08/stm8/pdk13/pdk14/pdk15/mos6502/mos65c02/f8/f8l TD- 4.5.24 #16456 (Mac OS X ppc)

4: Copy of the error message or incorrect output, or a clear description of the observed versus expected behavior.

The above example is taken from the Recommended Practice part of subsection 6.5.2 "Type qualifiers" in the _Optional TS.

Implementations that perform data-flow analysis are encouraged to produce a diagnostic
message if one operand has type pointer to optional-qualified type, the other operand has integer type, the operands are evaluated, and analysis cannot prove that no path exists on which the value of the pointer operand is a null pointer.
A diagnostic is encouraged regardless of whether the expression that is added to or subtracted from a pointer is an integer constant expression with value zero, because the referenced type of the result of the + or - operator is not optional-qualified.

SDCC does not produce the expected diagnostic message about the arithmetic operation (possibly the same issue as #4002) but instead produces what looks like a wrong diagnostic message about the return statement:

black.c:3: warning 196: pointer target lost _Optional qualifier

The diagnostic message about the return statement is a bug:

The return expressions of the amber, green and black functions do not violate a constraint on assignment because the + and - operators remove the _Optional qualifier that applied to the type of their pointer operand poi from the type of their result.

It is a variant of the same diagnostic message that would be produced if 'poi' instead pointed to a const-qualified type. Furthermore, making the return statement conditional on 'poi' being non-null does not prevent the diagnostic message about a lost qualifier from being produced.

[Christopher Bazley]

I am attaching a proposed bugfix for this issue, created with assistance from Codex in ChatGPT Work. I hope that it will prove acceptable when reviewed by SDCC's maintainers.

Roughly:
construct p + 0
mark it as temporarily preserved
early CSE leaves it intact
data-flow analysis determines whether p is non-null
issue or suppress the diagnostic
clear the preservation marker
rerun CSE, allowing p + 0 to be optimised to p

I have done the following testing:

• complete diagnostic-validation results (support/valdiag) for the default targets.
• complete runtime regression (support/regression) results for ucz80.

i.e.
make -j2
make -C support/regression clean-results
make -C support/regression -j2 test-ucz80
make -C support/valdiag clean
make -C support/valdiag -j2

Summary for 'ucz80': 0 failures, 36740 tests, 6407 test cases, 7347620 bytes, 1613638336 ticks
Summary for 'pdk15': 0 failures, 630 tests, 367 test cases, 0 bytes, 0 ticks

[Philipp Klaus Krause]

The approach would work, but it doesn't feel elegant to me. Treating pointer arithmetic on _Optional differently for so long feels (i.e. from AST to the early stages of processing the iCode) inelegant. Maybe we should keep all these +0 in AST, and eliminate them later? But that might have a code quality penalty (but if that is the case, we'd already see a code quality regression for adding zero to pointers to _Optional with this patch).

[Christopher Bazley]

Hi Philipp,

thanks for reviewing the candidate patch. I agree that the proposed approach was inelegant. The only justification I can give is that I thought that a minimal patch would be most appropriate for a bug fix. However, the solution to this bug is clearly related to the existing handling of p - 0 for pointers to optional-qualified types, so it seems better to have a holistic solution instead of treating this as an isolated bug.

Regarding optimal code generation, my initial reaction was that no one who cares about that should add zero to a pointer, but I soon realised that is unfair because zero can be hidden behind a macro, or it can be the result of a complex calculation. I now believe that an important principle should be that use of _Optional does not penalise performance.

The patch has been revised as follows:

• The previously-introduced flag iCode::optionalArithmetic has been removed.
• Existing code in algebraicOpts to turn addition of constant zero into an assignment or cast was refactored into a new function: foldAdditiveIdentity. Now algebraicOpts only optimises code where the non-zero operand does not have pointer or array type.
• A new optimisation function, foldPointerZeroArithmetic, is run after there is no longer any need to retain addition or subtraction of zero for the purpose of generating diagnostics. This new function also uses foldAdditiveIdentity.
• The existing workaround to set isSemDeref for cases like p - 0 in algebraicOpts was removed because it is now redundant.

I have tested the revised patch and obtained the following results:

Summary for 'ucz80': 0 failures, 36743 tests, 6408 test cases, 7344142 bytes, 1613409065 ticks
Summary for 'mcs51': 0 failures, 642 tests, 366 test cases, 0 bytes, 0 ticks
Summary for 'mcs51-large': 0 failures, 640 tests, 366 test cases, 0 bytes, 0 ticks
Summary for 'mcs51-stack-auto': 0 failures, 641 tests, 366 test cases, 0 bytes, 0 ticks
Summary for 'ds390': 0 failures, 628 tests, 366 test cases, 0 bytes, 0 ticks
Summary for 'z80': 0 failures, 631 tests, 366 test cases, 0 bytes, 0 ticks
Summary for 'z180': 0 failures, 630 tests, 366 test cases, 0 bytes, 0 ticks
Summary for 'r2k': 0 failures, 630 tests, 366 test cases, 0 bytes, 0 ticks
Summary for 'r4k': 0 failures, 630 tests, 366 test cases, 0 bytes, 0 ticks
Summary for 'sm83': 0 failures, 630 tests, 366 test cases, 0 bytes, 0 ticks
Summary for 'tlcs90': 0 failures, 630 tests, 366 test cases, 0 bytes, 0 ticks
Summary for 'hc08': 0 failures, 629 tests, 366 test cases, 0 bytes, 0 ticks
Summary for 's08': 0 failures, 629 tests, 366 test cases, 0 bytes, 0 ticks
Summary for 'mos6502': 0 failures, 629 tests, 366 test cases, 0 bytes, 0 ticks
Summary for 'stm8': 0 failures, 632 tests, 366 test cases, 0 bytes, 0 ticks
Summary for 'f8': 0 failures, 628 tests, 366 test cases, 0 bytes, 0 ticks
Summary for 'pdk13': 0 failures, 632 tests, 366 test cases, 0 bytes, 0 ticks
Summary for 'pdk14': 0 failures, 630 tests, 366 test cases, 0 bytes, 0 ticks
Summary for 'pdk15': 0 failures, 628 tests, 366 test cases, 0 bytes, 0 ticks

[Christopher Bazley]

Sorry, the patch attached above accidentally omitted the file support/regression/cases/tst_bug-4003.c that I think is required. Inbetween, I also became confused about the scope of this bugfix in relation to bug [#4002]. I think it's clearer to keep the two separated, even though the fix is in one patch and the patch for bug #4002 will contain only tests.

I attach an updated patch that contains the missing file, but none of the tests for bug #4002 that were mistakenly incorporated.

[Christopher Bazley]

Please find a rebased candidate fix attached. The implementation, tests and commit-message wording are unchanged from my last attachment; only patch context, line numbers and blob identifiers have changed. It applies to clean r16945 and has no known dependencies.

The revised fix for [bugs:#4006] will depend on this patch. The revised patch to fix [bugs:#4006] will move the genuine _Optional conversion checks into the AST and remove the late iCode checker. The tested series order is [bugs:#4072], [bugs:#4004], [bugs:#4005], [bugs:#4003] (this bug), then [bugs:#4006].

The full ucz80 regression passed with 0 failures, 36754 tests and 6413 test cases. After the final diagnostic-only changes to the patch for [bugs:#4006], the complete diagnostic-validation suite passed on all 19 targets and the focused ucz80 regression for this bug also passed.

[Philipp Klaus Krause]

Why is support/regression/cases/tst_bug-4003.c in the patch? That file is supposed to be generated from support/regression/tests/bug-4003.c as needed by the regression test infrastructure.

[Christopher Bazley]

Why is support/regression/cases/tst_bug-4003.c in the patch? That file is supposed to be generated from support/regression/tests/bug-4003.c as needed by the regression test infrastructure.

Ah, right, I did not understand that when I wrote (in a previous comment)

Sorry, the patch attached above accidentally omitted the file support/regression/cases/tst_bug-4003.c that I think is required.

[Philipp Klaus Krause]

Looks like I hadn't read through the thread properly, sorry for the noise.

[Christopher Bazley]

Here's another attempt at a viable patch.

[Philipp Klaus Krause]

The patch looks reasonably harmless, and definitely like the right thing to do. However, see on test fail (on my Debian GNU/Linux testing on amd64 laptop): support /regression/tests/bug3475630.c for the test-mcs51-medium target.

[Christopher Bazley]

Hi Philipp, please find a fixed version attached. (Still reviewing it.)

[Christopher Bazley]

I increasingly think that there should be an instruction that cannot be moved over flow control changes or removed by the optimiser, to keep both pointer dereferences and pointer arithmetic operations until diagnostics have been generated.

[Christopher Bazley]

The next candidate fix for this bug will depend on the fix for [bugs:#4109]

[Christopher Bazley]

The attached candidate patch should be applied after the candidate fix for [bugs:#4109] and the candidate fix for [bugs:#3952], before the candidate fixes for [bugs:#4005] and [bugs:#4006].

The proposed standalone fix for [bugs:#4109] preserves semantic-dereference diagnostics through dead-code elimination; this patch replaces that marker-preservation machinery with diagnostic-only iCode for both pointer arithmetic and dereferences, so folding zero offsets or &*p does not erase the checks. The checks are diagnosed after value analysis and removed before code generation.

Diagnostic tests passed on 16 configurations, and the full z80 regression suite passed both after this patch and after the subsequent fixes for [bugs:#4005] and [bugs:#4006]. Code-area sizes with and without _Optional matched on all 21 configurations tested.

[Maarten Brock]

@cs99cjb Please don't use [description](some url) to point to tracker items, etc. Simply use [#tracker-nr] for items in the same tracker or [bugs:#tracker-nr] for items in a different tracker. This way the site automatically adds the Related list, and cross references the items.