#4166 Z80 stores a structure member with its bytes reversed after r17036

[Christopher Bazley]

1: Sample code that reproduces the problem.

struct Item
{
  unsigned char tag;
  unsigned char padding[19];
  unsigned short first;
  unsigned short second;
  unsigned short third;
  unsigned char padding2[4];
  unsigned short last;
};

struct Table
{
  struct Item items[8];
};

static struct Table table;

static void
fill(unsigned short count, void *storage)
{
  struct Table *p = storage;

  for (unsigned char i = 0; i < count; ++i)
    {
      p->items[i].tag = i;
      p->items[i].first = i + 1;
      p->items[i].second = i + 2;
      p->items[i].third = i + 3;
      p->items[i].last = i + 4;
    }
}

int
main(void)
{
  fill(8, &table);
  for (unsigned char i = 0; i < 8; ++i)
    if (table.items[i].third != i + 3)
      return 1;
  return 0;
}

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

sdcc -mz80 --reserve-regs-iy --max-allocs-per-node 512 -S example.c

3: SDCC version tested.

Local builds with the shared compiler sources at r17035, r17036 and r17043, using the same Z80 backend and libraries.

The corresponding runtime regression test passes with r17035 and fails with r17036. The reduced runtime test also fails with r17043.

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

The program should return zero. The generated code stores the two bytes of third in reverse order. Instead of the expected values 3 through 10, the corresponding runtime test reads 768, 1024, 1280, 1536, 1792, 2048, 2304 and 2560.

For p->items[i].third = i + 3, the compiler calculates the low byte in B and the high byte in A, then stores A followed by B. Z80 requires the low byte to be stored first.

Adding --no-peep exposes the incorrect attempt to exchange the registers:

push af
ld a, b
ld b, a
pop af
ld (hl), a
inc hl
ld (hl), b

The pop af restores the original high byte into A, while B still contains the original low byte.

This was discovered in the r17038 random regression results. It can also be reproduced using the existing regression test:

make -C support/regression -j2 random-test-7589958616919012411 \
  TEST_STEMS=tst_bug-4003 \
  RESULTS_DIR=results-4003-check \
  TMP_DIR=gen-4003-check

With that configuration, support/regression/tests/bug-4003.c passes all 46 checks with r17035, but fails the eight checks of third with r17036. The separate support/valdiag/tests/bug-4003.c passes all 24 checks with both revisions.

The first failing adjacent revision is [r17036], which fixes [bugs:#4148] by allowing copy propagation through casts from pointer types. This appears to expose incorrect register-copy handling in the Z80 backend. The reproducer requires neither _Optional nor _Generic.

Replacing the incorrect register exchange in the generated assembly makes all 46 original runtime checks pass.

[Christopher Bazley]

To bypass the random configuration generator (e.g. on MacOS, because sed is not GNU's version):

cp -R support/regression/ports/ucz80-resiy \
      support/regression/ports/local-4166

printf '\nSDCCFLAGS += --nogenconstprop --noloopreverse --no-peep --nosidechannels --max-allocs-per-node 512\n' \
  >> support/regression/ports/local-4166/spec.mk

cd ~/sdcc-build

make -C support/regression -j2 test-local-4166 \
  TEST_STEMS=tst_bug-4003 \
  RESULTS_DIR=results-4166-local \
  TMP_DIR=gen-4166-local

[Christopher Bazley]

I don't have the time nor the expertise to investigate this today, but I did ask Codex to do so and it produced the attached candidate patch, with the following explanation:

The failure occurs in genCopy, including with --no-peep. A register-copy
cycle involving A attempts to use A as scratch storage while its original byte
is still needed. Save that byte on the stack and finish its move last.
This candidate changes no peephole rules.

Baseline: common compiler sources r17043, Z80 generator identical to r17044.
The full-suite test inventory is the retained r17041 inventory. The new #4166
test was generated and tested separately from its exact final source.

Completed validation:

• Eight failing online Z80 random configurations of bug-4003: each changes from
  8 failures / 46 checks to 0 / 46. This does not cover the other online Rabbit,
  eZ80 or Z80n configurations.
• New bug-4166 regression: 3 / 15 failures before, 0 / 15 after in ucz80-resiy.
  Final test also passes all 15 checks in ucz80, ucz80-undoc and host.
• Focused existing/new regression set: 0 failures, 93 checks, 13 cases in each
  of ucz80, ucz80-resiy and ucz80-undoc.
• Private tests of actual genCopy emission: 348 register permutations and
  live/dead flag combinations, repeated with mixed nonzero source/destination
  offsets; all pass in the simulator. These 696 emission cases are additional
  private checks, not shipped regression-framework test counts. Temporary
  compiler hooks were removed before building the final candidate compiler.

I hope this helps whoever picks up this bug. If not, feel free to ignore it.

[Philipp Klaus Krause]

Thanks. That fix would work. However, due to the importance of genCopy for efficient z80 code generation, I used a different approach for the fix. I used the test from your patch.

[Philipp Klaus Krause]

Fixed in [r17066].