#4202 STM8 removes an integer read through a volatile-qualified lvalue

[Christopher Bazley]

1: Sample code that reproduces the problem.

int object;
int test(void)
{
  object = 42;
  return *(volatile int *)&object;
}

Save as volatile-read-stm8.c.

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

sdcc -mstm8 --c1mode -o volatile-read-stm8.asm < volatile-read-stm8.c

The input contains no headers or macros. Add --no-peep to compare the output
without peephole optimisation.

3: SDCC versions tested.

SDCC 4.6.4 r17064, locally built from unmodified compiler sources; the official
SDCC 4.6.4 #17038 snapshot; and SDCC 4.2.0 #13081, all on Linux.

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

The generated function stores 42 and returns without reading object:

ldw x, #0x002a
ldw _object+0, x
ret

I expected the return expression to read object through its volatile-qualified
lvalue. With --no-peep, the read is present:

ldw x, #0x002a
ldw _object+0, x
ldw x, _object+0
ret

The same omission occurs in SDCC 4.2.0 #13081. A corresponding example storing
and then reading an int * through int *volatile * also loses its read in
r17064.

The problem is reproduced with unmodified r17064 sources, before the
Z80-family symbol-lookup candidate. It is not introduced by that candidate.

Possibly related to [bugs:#4156]. In this example, --no-peep retains the
required read; the loss can therefore be isolated to peephole optimisation.

[Christopher Bazley]

Attaching a candidate bugfix patch generated in discussion with Codex. It preserves volatile accesses through peephole replacements. Both lineElem_t and lineNode gain a isVolatileAccess flag. Before peephole rewriting, peepHole() examines each line’s associated iCode through lineNode::ic. A new helper function,hasVolatileAccess(), checks the existing operands’ isvolatile fields and, for pointer reads or writes, the pointed-to type’s volatile qualifier. If it finds a volatile access, peepHole() sets lineNode::isVolatileAccess. replaceRule() then preserves that information when replacing assembly lines, independently of their subsequent ic associations.

This is expected to fix [bugs:#4202], [bugs:#4203] and [bugs:#4204].

[Christopher Bazley]

Here's the same candidate volatile-preservation fix, rebased onto [r17071].

[Philipp Klaus Krause]

The idea looks good. Some details:

• Some ports use SET_VALUE_AT_ADDRESS instead of the POINTER_GET mechanism reusing '='. So hasVolatileAccess should handle that, too.
• This hasVolatileAccess/isVolatileAccess will give false positives on some lines; that is okay, but IMO should be reflected in the names (e.g. have something like "Might" in the name, like we do in the functions of the notUsed implementation in peep.c).

[Christopher Bazley]

Thanks for the review, @spth. Please take a look at this revised patch when you have time.

[Philipp Klaus Krause]

Looks good to me.

[Christopher Bazley]

Fixed by [r17075]