#3393 union-in-struct initalization issue

[Philipp Klaus Krause]

Recently added test qct/0051-inits fails when enabled in MakeList for pdk and mcs51. This test initializes a struct that contains a union.

[Howard M. Harte]

Root cause and a patch for this, still reproducible on the released 4.6.0 #16555 (and current trunk).

Root cause
promoteAnonStructs() (src/SDCCsymt.c) flattens the members of an anonymous union into the enclosing struct, giving them all the same offset and leaving no marker that they alias one another. createIvalStruct() (src/SDCCast.c) then walks that flattened field list and hands each member its own initializer, so the aliases consume the initializers belonging to the fields that follow the union. C11 6.7.9 makes an anonymous union a single member of the enclosing struct, so brace elision initializes it once, through its first member.

This is the counterpart to [#2643]: that one fixed the same confusion in printIvalStruct() on the copy path; createIvalStruct() on the iCode path was never fixed, which is why the failure shows up exactly on the targets [#2643] listed as unaffected.

Why it shows up on PDK and MCS51 specifically. Those targets (plus any other that emits initialized globals as generated code rather than through the __xinit_ copy) reach createIvalStruct() for a global. Targets using the copy path — ds390, hc08, s08, mos6502/65c02, z80 family, stm8, f8 — emit their data via printIval() and are masked for globals only. They are still wrong for local aggregates, which always go through createIvalStruct().

Reproducer 1 — the test in the ticket (support/regression/qct/0051-inits.c, no port guards):

sdcc -mmcs51 qct/0051-inits.c      # main() returns 3, should return 0
sdcc -mpdk14 qct/0051-inits.c      # same

Reading v out of simulator RAM after main() on pdk14, 4.6.0:

a=1  b=2  c=4  s.a=0  s.b=0        (expected 1, 2, 3, 4, 5)

The 4 meant for s.a landed on c, because d aliases it and consumed an initializer; s.a and s.b then had none left. Result byte 3 = the test's third check.

Reproducer 2 — local aggregate, no target or simulator needed. Returns 2 under SDCC, 0 under any hosted C compiler:

struct S { int a; union { int c; int d; }; int z; };

int main (void)
{
      struct S v = {1, 2, 3};
      if (v.a != 1) return 1;
      if (v.c != 2) return 2;   /* holds 3: d aliased c */
      if (v.z != 3) return 3;   /* holds 0: none left   */
      return 0;
}

Emitted code, 4.6.0 #16555. Two stores to one offset, nothing to the fields after the union:

mcs51:

mov ((_v + 0x0004) + 0),#0x03      ; c = 3
mov ((_v + 0x0004) + 0),#0x04      ; d = 4  <-- same offset
; _v+0x0006 and _v+0x0008 are never written

pdk14:

mov _v+4, a                        ; c = 3
 mov _v+4, a                        ; d = 4  <-- same offset

Runtime confirmation on 4.6.0 with the result byte written to a known address and read back in uCsim:

mcs51 res = 3, pdk14 res = 3 (0 = pass).

Fix (patch attached)
In createIvalStruct()'s brace-elision walk, skip the promoted siblings that alias storage already initialized in that walk, tracking the end offset of the last field initialized there. Bitfields are exempt, since several of them legitimately share one offset — so this does not address #2840.

With the patch, qct/0051-inits passes on mcs51 and pdk14, reproducer 2 returns 0, and the stores land at +0, +2 and +4.

Regression Test

 pdk14  0 failures / 14,794 tests / 6,292 cases
 mcs51 small / medium / small-stack-auto / large-stack-auto: 0 failures
 mcs51 large / huge: 3 failures — identical without the patch

[Maarten Brock]

The mentioned bug is fixed in [r16733].
However, when there are multiple structs with bitfileds in the union it still fails.
See regression test bug-3393.c

The whole idea to flatten anonymous members in promoteAnonStructs() seems like a bad choice.

[Howard M. Harte]

[r16733] fixes the reported case. Your commit message notes that it still fails when there are multiple structs with bitfields in the union — the case bug-3393.c carries under #if 0.

0001 of the set now on the patch tracker as [patches:#508] fixes that, and deletes the #if 0. On mcs51 the entire difference for struct S v = {1, 2, 3, 4} at file scope is one instruction:

before:  mov ((_gv + 0x0002) + 0),#0x04     ; the 4 lands at offset 2 -> c
after:   mov ((_gv + 0x0004) + 0),#0x04     ; the 4 lands at offset 4 -> z
         mov ((_gv + 0x0004) + 1),#0x00

The watermark compares byte offsets, so it has to exempt bitfields, since several of them legitimately share a byte. Comparing bit extents removes the need for the exemption: consecutive bitfields start where the previous one ended, whereas an alternative of a union starts where the union does.

What the set does not fix. A designated initializer naming a member of a nested anonymous union:

struct W { char a;
           union { struct { union { char p; struct { char x, y; }; };
                            char r; };
                   struct { char s, t, u; }; };
           char z; };
struct W v = {.a = 1, .t = 9, .z = 4};

Flattening splices both unions' members into one list, so a single union's members no longer sit together and a designator can name a member of a run the walk has not yet reached. 0003 fixes this for automatic storage, by asking overlapsInitialized() unconditionally rather than only when a designator had already been seen. At file scope it is unfixed on the targets that emit an initialized global as a data image: printIvalStruct() treats a run of promoted members as a whole union, which under nesting is only part of one, and drops the designated value. The image reads 01 00 00 00 04 where 01 00 09 00 04 is correct.

I could not find a local fix. The union's extent is not recoverable from the flat list — in the struct above, y sits at the byte where the first alternative ends, so a scan over member offsets stops there and never reaches s, t, u. Nothing in the flat list records that y belongs to the union. A fix appears to require the initializer walk to see the nesting, and since the designator reordering is keyed to the flat list as well (findStructFieldAt() walks ->fields, nextInitSlot() skips anonunionalias), that would reach printIvalStruct(), createIvalStruct(), reorderIlistFull() and the pic14 and pic16 copies, with the flat list retained for lookup and offsets.

That bears on your remark that flattening anonymous members in promoteAnonStructs() looks like a bad choice. These six work within the flattening and close the failures observable on the current design. If you intend to change that design, they are fixes to code you would be replacing, and it would be worth saying so before spending review time on them.

I can open a separate bug for the file-scope case with the reproducer above, or leave it tracked here — whichever you prefer.

[Maarten Brock]

I've been looking into a solution that does not flatten the anonymous members already quite a lot, but I could not yet find a good solution yet. So I am glad your [patches:#508] has been accepted by another developer in the meantime.