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

[Philipp Klaus Krause]

This looks like it would work, but also feels a bit like overkill.

After all the end effect is kind of the same as the "hack": union members are treated as volatile. And having that represented as volatile internally seems fine - after all when e.g. a long gets optimized into a BitInt(24) we don't differentiate it from something that was originally at BitInt(24) either.

It feels like the bug is just that the hack is applied too early - at a time when the type is still visible via _Generic. Shouldn't this be fine if we apply the "hack" later, after all the processing on the AST is done, but before we do any optimizations on the iCode? So what if we add the volatile around the time AST is transformed into iCode?

[Christopher Bazley]

Hi Philipp, thanks for reviewing the patch.

My concern about misrepresenting the volatility of the type was that qualified types have many other implications (other than for _Generic, that is, and even if some of those might be handled correctly, others might not. (Or future language features might bump into the same issues.)

It's possible to make test cases for _Generic pass simply by moving one compound assignment from compStructSize to geniCodeStruct, but I'm not yet convinced that is the right solution.

[Christopher Bazley]

With the simple fix outlined in my previous comment, assigning the array member of a volatile union to char * was accepted with no diagnostic. The same failure was reproduced on the unpatched mcs51 baseline.

union U { char a[4]; };
char *destination;

void f(volatile union U *u)
{
    destination = u->a;  /* Discards volatile. */
}

[Christopher Bazley]

I have a smaller alternative patch (6,492 bytes versus 10,577 for the extra-flag-based patch) but I still don't like it because it treats volatile as a special case for the purpose of type compatibility. I think the only principled solution is to treat all qualifiers the same, and diagnose misuse of all of them before the AST is transformed into iCode. I'm trying that approach.

[Christopher Bazley]

Hi Philipp, I hope this reduced patch, which is the first of a reworked patch series, will be useful.

[Philipp Klaus Krause]

Thanks. I've applied the patch in [r16953], since it IMO is a clear improvement over the current situation, and testing results look ok.
I agree that the result is still not ideal, since we still add the volatile qualifier before doing some checks for lost qualifiers in the original input, which feels like the wrong order.