#3952 No diagnostic message when type constraint on assignment violated

[Christopher Bazley]

1: Sample code that reproduces the problem.

float *itof(int *i)
{
    return i; // no warning
}

int main(void)
{
    int s = 0, *t;
    float *f = itof(&s);
    t = f; // no warning
    t = itof(f); // no warning
    return *f;
}

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

sdcc --std=c23 /work/bug.c

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

SDCC : mcs51/z80/z180/r2k/r2ka/r3ka/sm83/tlcs90/ez80_z80/z80n/r800/ds390/pic16/pic14/TININative/ds400/hc08/s08/stm8/pdk13/pdk14/pdk15/mos6502/mos65c02/f8 TD- 4.5.0 #15242 (Linux)

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

The C23 constraints on simple assignment (6.5.17.2) say:

the left operand has atomic, qualified, or unqualified pointer type, and (considering the type
the left operand would have after lvalue conversion) both operands are pointers to qualified
or unqualified versions of compatible types, and the type pointed to by the left operand has all
the qualifiers of the type pointed to by the right operand

The specification of "compatible types" (6.7.2) says:

Two types are compatible types if they are the same

The specification of "diagnostics" (5.1.1.3) says:

A conforming implementation shall produce at least one diagnostic message (identified in an
implementation-defined manner) if a preprocessing translation unit or translation unit contains
a violation of any syntax rule or constraint, even if the behavior is also explicitly specified as
undefined or implementation-defined.

This is an oversimplification, but regardless, float and int are not the same type.
https://www.open-std.org/JTC1/SC22/WG14/www/docs/n3220.pdf

I am aware that SDCC is not a conforming implementation of C, but from a practical perspective it seems to me that diagnosing assignment/initialisation of incompatible pointer types would be of great benefit to users.

[Christopher Bazley]

By the way, it makes no difference to the behaviour of SDCC if the example is altered to prevent the optimiser from determining the return value at compile time:

float *itof(int *i)
{
    return i; // no warning
}

int main(int argc, char *argv[])
{
    int s = argc, *t;
    float *f = itof(&s);
    t = f; // no warning
    t = itof(f); // no warning
    return *f;
}

[Benedikt Freisen]

Notes:

• case '=': in decorateType in SDCCast.c calls compareType in SDCCsymt.c, but only outputs diagnostics on complete incompatibility.
• compareType in this case forwards to comparePtrType, which in turn forwards to compareType
• compareType does not appear to distinguish between explicitly and implicitly castable

Maybe we need to introduce another return value for compareType.

[Philipp Klaus Krause]

This also affects function declaration checking when checking declaration vs. definition:

const int *f2(void);
volatile int *f2(void) {return 0;}

Does not give any diagnostic.

[Christopher Bazley]

Still not producing warnings with
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)

[Christopher Bazley]

I am attaching a proposed bugfix for this issue, created with assistance from Codex. I hope that it will prove acceptable when reviewed by SDCC's maintainers.
Tests and standard headers that provoked previously-undiagnosed and presumably unintentional constraint violations have also been updated to reduce noise in test logs.

I did the following testing of the proposed patch:

• 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, 7347646 bytes, 1613638288 ticks
Summary for 'mcs51': 0 failures, 656 tests, 370 test cases, 0 bytes, 0 ticks
Summary for 'mcs51-large': 0 failures, 654 tests, 370 test cases, 0 bytes, 0 ticks
Summary for 'mcs51-stack-auto': 0 failures, 655 tests, 370 test cases, 0 bytes, 0 ticks
Summary for 'ds390': 0 failures, 642 tests, 370 test cases, 0 bytes, 0 ticks
Summary for 'z80': 0 failures, 645 tests, 370 test cases, 0 bytes, 0 ticks
Summary for 'z180': 0 failures, 644 tests, 370 test cases, 0 bytes, 0 ticks
Summary for 'r2k': 0 failures, 644 tests, 370 test cases, 0 bytes, 0 ticks
Summary for 'r4k': 0 failures, 644 tests, 370 test cases, 0 bytes, 0 ticks
Summary for 'sm83': 0 failures, 644 tests, 370 test cases, 0 bytes, 0 ticks
Summary for 'tlcs90': 0 failures, 644 tests, 370 test cases, 0 bytes, 0 ticks
Summary for 'hc08': 0 failures, 643 tests, 370 test cases, 0 bytes, 0 ticks
Summary for 's08': 0 failures, 643 tests, 370 test cases, 0 bytes, 0 ticks
Summary for 'mos6502': 0 failures, 643 tests, 370 test cases, 0 bytes, 0 ticks
Summary for 'stm8': 0 failures, 646 tests, 370 test cases, 0 bytes, 0 ticks
Summary for 'f8': 0 failures, 642 tests, 370 test cases, 0 bytes, 0 ticks
Summary for 'pdk13': 0 failures, 645 tests, 370 test cases, 0 bytes, 0 ticks
Summary for 'pdk14': 0 failures, 643 tests, 370 test cases, 0 bytes, 0 ticks
Summary for 'pdk15': 0 failures, 641 tests, 370 test cases, 0 bytes, 0 ticks

[Christopher Bazley]

I think this patch needs rework. It is not a well thought-out complete solution.

I'm finding it hard to arrive at comprehensible solution because of this in compareTypeExact
(which is not the only dead code in that function):

#if 0
                      if (!compareTypeExact (exargs->type, checkValue->type, -1))
                        return 0;
#endif

[Christopher Bazley]

Please find a reworked version of the candidate fix for this bug attached. It has no known dependencies but two other patches depend on it:
the fix for bug #4005 will use the decay and pointer-checking code introduced by #3952.
the fix for bug #4006 will use #3952’s diagnostic result to avoid duplicate diagnostics, and array-decay handling from the fix for bug #4005 .

[Philipp Klaus Krause]

Its a big patch, hope to find time for it on Monday. The library fixes, though, look obviously good, so I've already picked them in [r16959].

[Philipp Klaus Krause]

I've looked at the changes to existing regression tests in support/regression. Apparently those just "fix" the code wherever a warning is emitted with [bugs:#3952] fixed.

We need to decide this case-by-case:

• Where the warning appears in what is essentially test infrastructure for the test, the approach is fine, and I applied the changes in [r16960].
• Where the change affects the actual tested code, we IMO need to keep the original code. That type punning of treating an unsigned long as a long or such via an implicit cast might be neither good nor standard-compliant code, but there is implementation-defined behaviour expected by users. For those, IMO, we just need to throw in another #pragma disable_warning, since the warning is expected, not an issue in the test. Or maybe we can keep the tested code by making a change in the infrastructure part instead (like I did for tests-bug-2320.c in the same commit).

[Christopher Bazley]

Hi Philipp,

Apparently those just "fix" the code wherever a warning is emitted with [bugs:#3952] fixed.

I don't feel strongly about it but I would like to point out that the simplest fix would have been to add casts as necessary, which is not what I decided to do everywhere.

My assumption was that you do not intentionally test implicitly undefined behaviour, that such behaviour is either unpredictable now or might become unpredictable in future, and therefore that any tests that violate type constraints on assignment do so by accident. That said, I am not familiar with what extension behaviour SDCC might specify that users might choose to exploit.

Unless my changes prevent test cases from exercising the cases they were designed to exercise, I don't see a problem. If they are tests specifically written to exercise constraint violations then that is of course different.

[Philipp Klaus Krause]

"Unless my changes prevent test cases from exercising the cases they were designed to exercise, I don't see a problem.." - then it becomes about knowing what "they were designed to exercise" - usually the regression tests named *-bug.c are derived from the user code that wasn't working as expected by the user, which means some user expected that code to work. Similarly for the gcc-* and the gte/*ones (except that we got them indirectly vie GCC.

[Christopher Bazley]

it becomes about knowing what "they were designed to exercise" - usually the regression tests named -bug.c are derived from the user code that wasn't working as expected by the user, which means some user expected that code to work. Similarly for the gcc- and the gte/*ones (except that we got them indirectly vie GCC.

Thanks for explaining. Please find a modified patch attached that uses #pragma disable_warning 244 to selectively disable diagnostic messages in bug-2632 and four GCC regression tests rather than changing types or adding explicit casts.

(replaced attachment at 09:58 on 5 Oct)

[Philipp Klaus Krause]

Thanks. I've picked those regression test changes for [r16962].

However, with the full patch applied I still see lots of warnings for some existing tests (e.g. tests/bug2320.c and tests/bug-3728.c for test-mcs51-small and test-ucz80), some of which look like false positives at first sight.

[Christopher Bazley]

However, with the full patch applied I still see lots of warnings for some existing tests (e.g. tests/bug2320.c and tests/bug-3728.c for test-mcs51-small and test-ucz80), some of which look like false positives at first sight.

I hope this helps.

[Philipp Klaus Krause]

Thanks. For now, I've merged most of the regression test changes in [r16970]. For the changes in src/SDCCsymt.c, I see a merge conflict.

Regarding the fix for support/regression/tests/p99-conformance.c: I see the problem in there, and will merge the workaround wit the rest of the patch. That test code was written by Jens Gustedt for his P99 library, you might want to report the bug to him to also get it fixed in upstream P99.