#4072 _Generic incorrectly sees an unqualified union array member as volatile

[Christopher Bazley]

1: Sample code that reproduces the problem.

union U
{
  char m[64];
  int i;
};

void unqualified(union U *p)
{
  _Static_assert(_Generic(p->m,
                          char *: 1,
                          default: 0),
                 "unqualified union member acquired volatile");
}

void qualified(volatile union U *p)
{
  _Static_assert(_Generic(p->m,
                          volatile char *: 1,
                          default: 0),
                 "volatile union member lost volatile");
}

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

sdcc --std=c11 -S union-field-volatile-generic.c

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

SDCC : mcs51 TD- 4.6.2 #16838 (Linux)
published under GNU General Public License (GPL)

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

SDCC produces:

union-field-volatile-generic.c:11: warning 215: static assertion failed: "unqualified union member acquired volatile"

No diagnostic is produced for the second assertion.

The expression p->m designates an array member of an unqualified union. Array-to-pointer conversion should therefore produce char *. It should not produce volatile char *, because neither the union nor its member was declared as volatile.

Conversely, when p points to a volatile-qualified union, the member access is volatile-qualified and array-to-pointer conversion should produce volatile char *. The second assertion confirms that SDCC already handles that case as expected.

GCC accepts both assertions with -std=c11 -pedantic-errors.

An internal SDCC implementation detail is thus exposed through the C type system. In SDCCsymt.c, SDCC currently marks every union field volatile:

SPEC_VOLATILE(loop->etype) |= (sdef->type == UNION ? 1 : 0);

There was an earlier discussion of this implementation strategy in bug [#2501]. Philipp noted that all union members were considered volatile by SDCC and questioned whether making them volatile was appropriate. Maarten described it as a hack intended to ensure that modifying one union member invalidates information about the others.

Bug [#2501] was about a separate dead-code-elimination/register-allocation failure and was closed after that failure was fixed. As far as I can tell, it did not address the fact that the artificial qualifier becomes observable through language features such as _Generic.

The internal requirement to treat union accesses conservatively should not change the type of an unqualified union member.

[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.

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, 7344747 bytes, 1609270869 ticks

Summary for 'mcs51': 0 failures, 642 tests, 364 test cases, 0 bytes, 0 ticks

Summary for 'mcs51-large': 0 failures, 640 tests, 364 test cases, 0 bytes, 0 ticks

Summary for 'mcs51-stack-auto': 0 failures, 641 tests, 364 test cases, 0 bytes, 0 ticks

Summary for 'ds390': 0 failures, 628 tests, 364 test cases, 0 bytes, 0 ticks

Summary for 'z80': 0 failures, 631 tests, 364 test cases, 0 bytes, 0 ticks

Summary for 'z180': 0 failures, 630 tests, 364 test cases, 0 bytes, 0 ticks

Summary for 'r2k': 0 failures, 630 tests, 364 test cases, 0 bytes, 0 ticks

Summary for 'r4k': 0 failures, 630 tests, 364 test cases, 0 bytes, 0 ticks

Summary for 'sm83': 0 failures, 630 tests, 364 test cases, 0 bytes, 0 ticks

Summary for 'tlcs90': 0 failures, 630 tests, 364 test cases, 0 bytes, 0 ticks

Summary for 'hc08': 0 failures, 629 tests, 364 test cases, 0 bytes, 0 ticks

Summary for 's08': 0 failures, 629 tests, 364 test cases, 0 bytes, 0 ticks

Summary for 'mos6502': 0 failures, 629 tests, 364 test cases, 0 bytes, 0 ticks

Summary for 'stm8': 0 failures, 632 tests, 364 test cases, 0 bytes, 0 ticks

Summary for 'pdk14': 0 failures, 629 tests, 364 test cases, 0 bytes, 0 ticks

Summary for 'pdk15': 0 failures, 627 tests, 364 test cases, 0 bytes, 0 ticks

[Maarten Brock]

As far as I can tell, it did not address the fact that the artificial qualifier becomes observable through language features such as _Generic.

Note that in 2016 SDCC did not support _Generic at all.

[Maarten Brock]

Christopher,

Please try to use [ # (ticket number) ] or [ bugs:# (ticket number) ] when referencing other bugs as this creates automatic references between the tickets. See also the Formatting Help on the left.

I took a peek at the patch and have some comments.

I wonder if the test should be in /support/valdiag/tests/. AFAIK that is for testing that SDCC outputs the expected warning or error. Though I must admit I have never used it yet. The presented test case does not output any diagnostic.

So I think this test should go in /support/regression/tests/ and it could use a second test in
/support/valdiag/tests/ to check for valid and invalid diagnostics.

Other than that the patch looks good to me.

Maarten

[Philipp Klaus Krause]

In general support/regression is a more powerful test infrastructure. We used it to ensure that stuff compiles, and has the expected behaviour (via the ASSERT macro).
The infrastructure in support valdiag is what we use for diagnostics. Basically, it can check that for every line the behaviour is either no diagnostic, warning or error. That is something we can't do in support/regression.
Normally, when not explicitly testing diagnostics, we use support/regression.

[Christopher Bazley]

Hi Philipp & Maarten,

Thanks for reviewing my patch. I have reworked the test to use ASSERT instead of _Static_assert and moved it as requested. The remainder of the patch is identical to the previous version. I also added a commit message, which I also reproduce below:

Fix #4072: _Generic sees union members as volatile

Union membership must not add volatile to a member's type
because qualifiers can become visible via _Generic.
Previously, it did so because of a hack intended to ensure
that modifying one union member invalidates information
about others. This hack did not even achieve its aim in the
case of members of pointer type, because the referenced
type was made volatile instead of the pointer type.

Often, the bug went unnoticed, because the first argument
to _Generic is treated as if it undergoes lvalue conversion
(which removes qualifiers); however, when a union contained
a member of array or pointer type, lvalue conversion did
not remove qualifiers from the referenced type.

A new flag, sym_link::volatileAccess, indicates that an
object should be treated as volatile only for internal
purposes.  Instead of mutating the type of union members
in compStructSize(), the new flag is set.  structElemType()
propagates its value to members accessed through a union,
and aggregateToPointer() preserves its value when an array
is converted to a pointer.

SDCCsymt.h already defined IS_VOLATILE() as another name
for isVolatile(), but the macro was not used consistently.
Switch the macro to alias a new function,
isVolatileAccess().  It behaves like isVolatile()
except that it also returns true if volatileAccess is set
for the queried sym_link, or for any sym_link in an
associated chain of array type derivations.

Summary for 'ucz80': 0 failures, 36743 tests, 6408 test cases, 7345555 bytes, 1609295246 ticks

[Christopher Bazley]

A new issue was discovered: the previous version of this patch had the side-effect of causing the mcs51 compiler to crash because of a register spill-location problem. It has therefore been updated to set the new sloc->type->volatileAccess flag to false in createStackSpil, just like the existing SPEC_VOLATILE was already zeroed.
(I did not find this during my previous testing because I did not 'make -C device/lib clean'.)

Results of retesting are below.

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

valdiag results:
Summary for 'mcs51': 0 failures, 641 tests, 363 test cases, 0 bytes, 0 ticks
Summary for 'mcs51-large': 0 failures, 639 tests, 363 test cases, 0 bytes, 0 ticks
Summary for 'mcs51-stack-auto': 0 failures, 640 tests, 363 test cases, 0 bytes, 0 ticks
Summary for 'ds390': 0 failures, 627 tests, 363 test cases, 0 bytes, 0 ticks
Summary for 'z80': 0 failures, 630 tests, 363 test cases, 0 bytes, 0 ticks
Summary for 'z180': 0 failures, 629 tests, 363 test cases, 0 bytes, 0 ticks
Summary for 'r2k': 0 failures, 629 tests, 363 test cases, 0 bytes, 0 ticks
Summary for 'r4k': 0 failures, 629 tests, 363 test cases, 0 bytes, 0 ticks
Summary for 'sm83': 0 failures, 629 tests, 363 test cases, 0 bytes, 0 ticks
Summary for 'tlcs90': 0 failures, 629 tests, 363 test cases, 0 bytes, 0 ticks
Summary for 'hc08': 0 failures, 628 tests, 363 test cases, 0 bytes, 0 ticks
Summary for 's08': 0 failures, 628 tests, 363 test cases, 0 bytes, 0 ticks
Summary for 'mos6502': 0 failures, 628 tests, 363 test cases, 0 bytes, 0 ticks
Summary for 'stm8': 0 failures, 631 tests, 363 test cases, 0 bytes, 0 ticks
Summary for 'f8': 0 failures, 627 tests, 363 test cases, 0 bytes, 0 ticks
Summary for 'pdk13': 0 failures, 630 tests, 363 test cases, 0 bytes, 0 ticks
Summary for 'pdk14': 0 failures, 628 tests, 363 test cases, 0 bytes, 0 ticks
Summary for 'pdk15': 0 failures, 626 tests, 363 test cases, 0 bytes, 0 ticks

~/sdcc-git-mirror $ ./bin/sdcc \

-Idevice/include \
-Idevice/include/mcs51 \
--model-small --nostdinc --std-c23 \
-c device/lib/_ulonglong2fs.c \
-o /tmp/_ulonglong2fs.rel

No ICE anymore.

Attaching the updated patch.