#4004 Missing diagnostic when qualifier is discarded from pointer target on function call

[Christopher Bazley]

1: Sample code that reproduces the problem.

#include <stdlib.h>

// & yields a pointer to non-optional-qualified type
#define optional_cast(p) ((typeof(&*(p)))(p))

// const-qualified referenced type of s is accidental
void free_str_2(_Optional const char *s)
{
  free(optional_cast(s)); // constraint violation
}

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

sdcc --stack-auto --std=c23 -c free_str_2.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 Semantics part of the subsection "Typeof specifiers" in the _Optional TS. Specifically:

Because s does not have a variably modified type in the macro-replaced expression (typeof(&(s)))(s)) that results from invocations of the optional_cast macro, the operand of typeof is not evaluated, therefore no diagnostic message is recommended for the lvalue (s) .

The address-of operator is defined as removing the _Optional qualifier from the type of its operand but it must not remove any other qualifiers. The expected type of the argument expression in the free function call is therefore const char *, which violates a constraint on assignment (assuming the free function has the ISO standard parameter type).

SDCC does not report that the const qualifier was lost from the pointer target during the assignment implicit in the function call. That seems like a bug, because the ISO C standard requires a diagnostic message to be produced for every constraint violation.

[Christopher Bazley]

After further investigation, I don't think this bug is specific to _Optional:

void f(void *t);
void g(char *t);
void h(int *t);

void foo(void)
{
  volatile char *t = 0;
  f(t); // undiagnosed violation
  g(t); // undiagnosed constraint violation
  h(t); // undiagnosed violation

  const char *s = 0;
  f(s); // undiagnosed constraint violation
  g(s); // diagnosed constraint violation
  h(s); // undiagnosed violation
}

[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. This patch is relatively simple compared to some others.

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 'mcs51': 0 failures, 651 tests, 367 test cases, 0 bytes, 0 ticks
Summary for 'mcs51-large': 0 failures, 649 tests, 367 test cases, 0 bytes, 0 ticks
Summary for 'mcs51-stack-auto': 0 failures, 650 tests, 367 test cases, 0 bytes, 0 ticks
Summary for 'ds390': 0 failures, 637 tests, 367 test cases, 0 bytes, 0 ticks
Summary for 'z80': 0 failures, 640 tests, 367 test cases, 0 bytes, 0 ticks
Summary for 'z180': 0 failures, 639 tests, 367 test cases, 0 bytes, 0 ticks
Summary for 'r2k': 0 failures, 639 tests, 367 test cases, 0 bytes, 0 ticks
Summary for 'r4k': 0 failures, 639 tests, 367 test cases, 0 bytes, 0 ticks
etc.

[Philipp Klaus Krause]

The patch looks good to me (except for the naming of the parameters - I'd prefer "target" instead of "new", "source" instead of "orig" to keep the terminology consistent, but I can change that when applying).

For now, IMO we should wait a few days for the discussion on sdcc-devel on LLM use to settle.

[Christopher Bazley]

Retesting this patch revealed that it triggers a flood of 'volatile pointer lost' warnings from tests in the regression suite. There is also an open question about whether treating pointers specially is sufficient, or whether the conditions should be updated to include array types too.

[Christopher Bazley]

The attached candidate patch has now been significantly reworked and retested. It no longer triggers a flood of warnings from existing tests, and it now produces the correct behaviour in cases of array-to-pointer decay. Please consider it for incorporation into SDCC, but ensure that it is applied after the fix for bug #4072, on which it depends.

Commit message follows:

Fix bug #4004: No diagnostic when qualifier is discarded on
 function call

At argument-type-checking time, SDCC still represents an
argument of array type as an array because array-to-pointer
conversion is performed later during iCode generation.
Consequently, it is necessary to derive the converted
(pointer) type temporarily so that lost-target-qualifier
diagnostics use the type required by the language without
changing the AST used by the rest of the compiler.

Amongst the new test cases, union-member cases are added
to distinguish genuine volatile qualification from the
artificial volatility that caused bug #4072.

Existing tests were updated to prevent a flood of mandatory
diagnostic message from tests that already violated the
constraint on assignment.

regression test results:
Summary for 'ucz80': 0 failures, 36743 tests, 6408 test cases, 7347903 bytes, 1609599005 ticks

valdiag test results:
Summary for 'mcs51': 0 failures, 659 tests, 369 test cases, 0 bytes, 0 ticks
Summary for 'mcs51-large': 0 failures, 657 tests, 369 test cases, 0 bytes, 0 ticks
Summary for 'mcs51-stack-auto': 0 failures, 658 tests, 369 test cases, 0 bytes, 0 ticks
Summary for 'ds390': 0 failures, 645 tests, 369 test cases, 0 bytes, 0 ticks
Summary for 'z80': 0 failures, 648 tests, 369 test cases, 0 bytes, 0 ticks
Summary for 'z180': 0 failures, 647 tests, 369 test cases, 0 bytes, 0 ticks
Summary for 'r2k': 0 failures, 647 tests, 369 test cases, 0 bytes, 0 ticks
Summary for 'r4k': 0 failures, 647 tests, 369 test cases, 0 bytes, 0 ticks
Summary for 'sm83': 0 failures, 647 tests, 369 test cases, 0 bytes, 0 ticks
Summary for 'tlcs90': 0 failures, 647 tests, 369 test cases, 0 bytes, 0 ticks
Summary for 'hc08': 0 failures, 646 tests, 369 test cases, 0 bytes, 0 ticks
Summary for 's08': 0 failures, 646 tests, 369 test cases, 0 bytes, 0 ticks
Summary for 'mos6502': 0 failures, 646 tests, 369 test cases, 0 bytes, 0 ticks
Summary for 'stm8': 0 failures, 649 tests, 369 test cases, 0 bytes, 0 ticks
Summary for 'f8': 0 failures, 645 tests, 369 test cases, 0 bytes, 0 ticks
Summary for 'pdk13': 0 failures, 648 tests, 369 test cases, 0 bytes, 0 ticks
Summary for 'pdk14': 0 failures, 646 tests, 369 test cases, 0 bytes, 0 ticks
Summary for 'pdk15': 0 failures, 644 tests, 369 test cases, 0 bytes, 0 ticks

[Philipp Klaus Krause]

I've started reviewing this, but most likely won't finish today.

[Philipp Klaus Krause]

So basically, this patch does the checking already on the AST instead of later on the iCode. Which, I think makes sense, since the AST is closer to what the user wrote. And it looks like it does the right thing for access qualifiers on the immediate pointer target. I've done some testing, and there are still issues with qualifiers further down the type chain, and for qualifiers other than access qualifiers:

void f(void *t);
void g(char *t);
void h(int *t);

void f2(void **t);
void g2(char **t);
void h2(int **t);

void set_space(void);

__addressmod set_space space;

volatile char *t = 0; // access qualifier
const char *s = 0;    // access qualifier
__far char *u = 0;    // intrinsic named address space qualifier for an address space not contained in the generic address space (for Rabbits, TLCS-90, eZ80).
space char *v = 0;    // non-intrinsic named address space qualifier

void foo(void)
{

  f(t); // undiagnosed - diagnosed with current patch
  g(t); // undiagnosed - diagnosed with current patch
  h(t); // undiagnosed - diagnosed with current patch

  f(s); // undiagnosed - diagnosed with current patch
  g(s); // diagnosed
  h(s); // undiagnosed - diagnosed with current patch


  f(u); // undiagnosed - dangerous!
  g(u); // undiagnosed - dangerous!
  h(u); // undiagnosed - dangerous!

  f(v); // diagnosed
  g(v); // diagnosed
  h(v); // diagnosed


  f2(&t); // undiagnosed
  g2(&t); // undiagnosed
  h2(&t); // undiagnosed

  f2(&s); // undiagnosed
  g2(&s); // undiagnosed
  h2(&s); // undiagnosed


  f2(&u); // undiagnosed - dangerous!
  g2(&u); // undiagnosed - dangerous!
  h2(&u); // undiagnosed - dangerous!

  f2(&v); // undiagnosed - dangerous!
  g2(&v); // undiagnosed - dangerous!
  h2(&v); // undiagnosed - dangerous!
}

The most complicated are the intrinsic named address space qualifiers. Users probably don't want warnings where qualifiers change in a way that the immediate target is in an address space that contains the address space of the source. But we'd still need those warnings if such a change happens further down the type chain.

Still the patch is already an improvement over the current situation. But it doesn't fix all cases from the title of this bug report.

[Christopher Bazley]

Hi Philipp, thanks very much for the review.

Sorry that the fix is incomplete. I am mainly focusing on improving ISO C conformance. I am not familiar with the semantics of the non-standard qualifiers (other than _Optional), therefore I don't feel qualified to comment on what their behaviour should be.

If you are happy with the direction of the current patch, maybe the best thing to do would be to merge it as-is and leave treatment of non-standard qualifiers to another patch?

By the way, did you see that this patch depends on the candidate fix for bug #4072 ?

[Christopher Bazley]

Hi Philipp,

Please find a revised candidate fix attached, which still depends on the fix for bug #4072. The previous implementation has been extended to diagnose incompatible qualifiers further down the type chain, including in function parameter and return types. Top-level parameter qualifiers are ignored when comparing function types, as are qualifiers on return types.

The tests now cover deeper indirection, pointer-to-array versus pointer-to-pointer incompatibility, and qualification differences in function types. Cases whose expected diagnostic messages would change under N3449 are grouped separately and highlighted by comments. Workarounds for bugs #4104 and #4105 are also highlighted by comments.

Named address-space compatibility is not yet addressed, but I think that handling the other qualifiers correctly would be a worthwhile step forward. Personally, I'd treat that as a separate bug or feature request that comes with its own sample code to reproduce any known issues.

[Philipp Klaus Krause]

Since the new functions operate on types, they should go in SDCCsymt.c, not SDCCast.c (even if all calls for now are from SDCCast.c), since in general symbol and type handling is done in SDCCsymt.c. I guess we can also drop the "Std" in the name of compatibleStdQualifiers, since we want to extend this to handle all qualifiers later. Preferably put a // TODO: Handle named qualifiers for named address spaces where their handling is missing for now.

Regarding named address spaces qualifiers: I agree that we can consider that to be a separate issue, for which I'd open a ticket once this one is closed.

[Christopher Bazley]

Thanks. Here's another attempt at a viable patch. Type-only helpers are moved to SDCCsymt.c.

[Philipp Klaus Krause]

New ticket for qualifiers for intrinsic named address spaces: [bugs:#4106].

[Philipp Klaus Krause]

Thanks. Fixed in [r16956] via your patch.