#4149 _Optional analysis incorrectly accepts repeated volatile structure member reads

[Christopher Bazley]

1: Sample code that reproduces the problem.

struct S
{
  char padding;
  _Optional int *p;
};

int read_value(volatile struct S *s)
{
  if (s->p)
    return *s->p;
  return 0;
}

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

sdcc -mz80 --std-c99 -c volatile-member.c

3: SDCC version tested

SDCC trunk r16999.

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

There is no warning about *s->p whereas I expected warning 355. Access through a pointer to a volatile structure makes a pointer member volatile. Use of the pointer member's value in the if statement's controlling expression and the dereference of the same pointer in the return expression read the value of that member separately; the second read may yield null even if the first did not. The guarding if statement therefore does not establish that the value used for dereference is non-null.

The same omission occurs with z80, mcs51, r2k, stm8, s08 and pdk14.

Removing padding makes trunk warn on all six targets. This layout-dependent warning masks the underlying mishandling of volatile qualification. Fixing the separate first-member problem reported in [bugs:#4148] also removes that warning unless the volatile issue is fixed.

[Christopher Bazley]

Fixing the separate first-member problem reported in [bugs:#4148] also removes that warning unless the volatile issue is fixed.

In other words, we do want a diagnostic message to be produced for the sample code, but we do not want it to be produced for the wrong reason.

[Christopher Bazley]

Please consider the attached candidate patch.

[Philipp Klaus Krause]

While this doesn't apply for me against current trunk, I applied the src/SDCCicode.c changes manually to test, and it looks good to me. I did a few quick tests using hand-written testcases around struct with volatile members, or members that are pointers to volatile, and did not notice a regression wjen reading the resulting .asm and looking at iCode and AST dumps.

In general, I am worried about SDCC bugs wrt. volatile, since they tend to be hard to test for. So I welcome this testing opportunity _Optional gives us here.

If you can think of further tests relating to volatile that we could get with _Optional, please look into that (though that might be beyond the scope of this ticket).

[Christopher Bazley]

@spth you were right about this:

In general, I am worried about SDCC bugs wrt. volatile, since they tend to be hard to test for. So I welcome this testing opportunity _Optional gives us here. If you can think of further tests relating to volatile that we could get with _Optional, please look into that (though that might be beyond the scope of this ticket).

Codex claims to have found one other similar bug: s->a[1] loses its second read.

struct S
{
  char padding;
  _Optional int *a[2];
};

int read_value(volatile struct S *s)
{
  if (s->a[1])
    return *s->a[1];
  return 0;
}

I have not yet confirmed it or reported it separately.

[Christopher Bazley]

@spth I have since reported the newly-discovered bug in compilation of the sample code above as [bugs:#4161].

[Christopher Bazley]

Fixed by [r17035].

[Philipp Klaus Krause]

I see a code size regression for Rabbit ports.

I've looked into one affected line in the Dhrystone benchmark, line 105 in dhry_1.c:

Ptr_Glob = /*(Rec_Pointer) malloc (sizeof (Rec_Type))*/ &malloc_2;

Ptr_Glob is a pointer to a struct with three members, one of them a union. Previously, a peephole optimizer rules was able to optimize out a read from the memory location at which this pointer is stored:

;dhry_1.c:105: Ptr_Glob = /*(Rec_Pointer) malloc (sizeof (Rec_Type))*/ &malloc_2;
;   skipping iCode since result will be rematerialized
;   genCast
;   genMove_o size 2 result type 10 source type 7 a_dead 1 hl_dead 1 de_dead 1 iy_dead 1 f_dead 1
;   genMove_o size 2 result type 2 source type 7 a_dead 1 hl_dead 1 de_dead 0 iy_dead 1 f_dead 1
    ld  hl, #_malloc_2
;   genMove_o size 2 result type 10 source type 2 a_dead 1 hl_dead 1 de_dead 0 iy_dead 1 f_dead 1
;dhry_1.c:107: Ptr_Glob->Ptr_Comp                    = Next_Ptr_Glob;
;   genCast
;   genMove_o size 2 result type 2 source type 10 a_dead 1 hl_dead 1 de_dead 1 iy_dead 1 f_dead 1
; common peephole 9b reused value still in hl.
    ld  (_Ptr_Glob), hl

Now, this read is kept. Apparently the heuristics the peephole optimizer uses are not good enough to prove that the pointer is not volatile.

Don't know if this issue is worth fixing, but I think we should at least look into it a bit. There are more code size regressions than that one line, but this is the only one I looked into a bit more so far.

P.S.: We see the increase in code size e.g. at https://sourceforge.net/p/sdcc/code/HEAD/tree/trunk/sdcc-extra/historygraphs/dhrystone-r4k-size.svg

[Christopher Bazley]

Hi @spth, is this candidate fix for the regression any use? I asked Codex to investigate and it has produced a patch.

The attached candidate allows exact parenthesised symbol names to reach the operand lookup, after checking the matched instructions for volatile accesses. On r2k it restores rule 9b at the Dhrystone location you identified and reduces the assembled dhry_1.c code area by three bytes. The reduced case, explicit volatile accesses, a volatile cast and the original #4149 example were checked before and after on all six Rabbit targets and Z80. This has not been tested with the full regression suite, and the other size regressions remain uninvestigated.

For Z80-family targets, an operand of the exact form (_name) can reach the existing operand lookup. Register-indirect operands, address expressions and unresolved names retain the existing conservative fallback.

Before allowing this lookup, the candidate checks the matched instructions for volatile operands and volatile pointer accesses. This also protects accesses through volatile casts to objects whose declarations are non-volatile. The #4149 fix and artificial union volatility are retained.

[Philipp Klaus Krause]

IMO, relying on line->ic is too risky, since after multiple application of peephole optimizer rules that information might have lost too much meaning. Could we try to look up the symbol in the symbol table, like we do at call instructions (to find out which registers hold register parameters, e.g. in src/z80/peep.c) instead?

[Christopher Bazley]

How about this version?

For Z80-family targets, an exact (_name) operand is resolved with
findSym(SymbolTab, NULL, name_without_leading_underscore), as for direct
calls. The assembly name must also match sym->rname. The lookup checks the
object's outer volatility using isVolatile(sym->type), so a non-volatile
pointer to volatile data remains distinct from a volatile pointer object.

Register-indirect accesses, address expressions, mismatched assembly names and
unresolved symbols retain the existing function-wide conservative fallback.
No new lookup relies on line->ic. The pre-existing paths for other operand
forms and targets are unchanged. The #4149 fix and artificial union volatility
are retained.

provenance-test.c includes the revised compiler source and embeds the exact
notVolatileVariable function from the preceding candidate under a different
name. It uses real symbol-table lookup and the compiler's actual
reassociate_ic routine.

The test folds an address materialisation followed by a byte load into a direct
byte load, with the address register dead. The source lines have different
iCodes. Neither matches the replacement, so reassociate_ic leaves the new
line without an iCode association. This is a constructed host-side test of the
real helper, not a C program found to trigger a particular shipped peephole rule.

Assembly operand and associated information	Original candidate	Revised candidate	Expected
Non-volatile object; missing iCode	Retain access	Allow optimisation	Allow
Non-volatile object; unrelated volatile iCode	Retain access	Allow optimisation	Allow
Non-volatile object; unrelated ordinary iCode	Retain access	Allow optimisation	Allow
Volatile object; unrelated ordinary iCode	Retain access	Retain access	Retain
Volatile object; missing iCode	Retain access	Retain access	Retain
Unresolved symbol; missing iCode	Retain access	Retain access	Retain
Volatile-cast address expression; missing iCode	Retain access	Retain access	Retain
Register-indirect operand; missing iCode	Retain access	Retain access	Retain

The first three cases demonstrate that the original candidate is insufficient
to recover valid optimisations when provenance is lost or unrelated. They do
not demonstrate a miscompilation or prove that an existing rule reaches every
constructed state. No unsafe C reproducer for the original candidate was found.

lookup-test.c additionally checks pointer-object versus pointee volatility,
mismatched assembly names, malformed and overlong operands, unknown names,
and fallback behaviour with and without a current function using volatile
accesses. Both host-side test programs pass.

[Philipp Klaus Krause]

Looks good to me, and works fine for me (for Dhrystone, and the ucz80 and ucr3ka regression tests pass). This should solve the found issue for the z80-related ports.

We probably want this for some other ports, such as stm8, too.

P.S.: possibly introduce a helper function that find the symbol from const char * pointer into an asm line (could return NULL if not found), which we could then use both in the z80 path, in the stm8 path, and later also in the peep.c files where we look up function symbols to determine the register parameters.

[Christopher Bazley]

Looks good to me, and works fine for me (for Dhrystone, and the ucz80 and ucr3ka regression tests pass). This should solve the found issue for the z80-related ports.

Thanks for the review, @spth. The approved patch is committed as [r17071].

We probably want this for some other ports, such as stm8, too.
P.S.: possibly introduce a helper function that find the symbol from const char * pointer into an asm line (could return NULL if not found), which we could then use both in the z80 path, in the stm8 path, and later also in the peep.c files where we look up function symbols to determine the register parameters.

I have a pending follow-up patch for that. During the development and testing of that follow-up patch, [bugs:#4202], [bugs:#4203] and [bugs:#4204] were discovered.

[Christopher Bazley]

We probably want this for some other ports, such as stm8, too.
P.S.: possibly introduce a helper function that find the symbol from const char * pointer into an asm line (could return NULL if not found), which we could then use both in the z80 path, in the stm8 path, and later also in the peep.c files where we look up function symbols to determine the register parameters.

@spth please review the attached patch to implement the above requested change. It depends on the unreviewed patch to fix [bugs:#4202] and friends.

[Christopher Bazley]

Attaching the same patch rebased on the updated patch to fix [bugs:#4202] and friends.

[Philipp Klaus Krause]

With your patch for [bugs:#4202] in, I think we could just ditch the old funcUsesVolatile and global_not_volatile, and use mightBeVolatileAccess or-ed over the lines in question instead? That should make notVolatile conditions in peephole optimizer rules even more accurate.

[Christopher Bazley]

@spth It wasn't quite that straightforward. Codex found a regression in code generation for STM8, caused by removing funcUsesVolatile:

char test(char *p)
{
  *p = 42;
  return *(volatile char *)p;
}

This STM8 counterexample loses its qualification in optimizeFinalCast in src/SDCCopt.c, just before register allocation. This optimisation replaces a pointer cast's single use with the original pointer operand, then deletes the cast. It checks pointer representation and address space, but did not check the volatility of the pointed-to type. The fix retains a cast whenever source and destination pointers differ in the volatility of the immediately pointed-to type. It handles casts that add or remove volatile, including the volatility of a pointer object accessed through a pointer-to-pointer.

The attached patch is therefore a necessary prerequisite for the changes that you suggested. I think it's relatively safe on its own because it merely has the potential to suppress an optimisation.

[Christopher Bazley]

@spth Here's the follow-up patch to make the changes that you suggested.

[Philipp Klaus Krause]

Clearly an improvement, though that parsing with ISCHARALNUM feels a bit crude - it might surprise users that some optimization gets disabled when they use non-ascii characters in identifiers. Don't know if it is worth putting more effort into parsing symbol names here, though.

[Christopher Bazley]

Thanks. Merged as [r17081].

[Philipp Klaus Krause]

Still, could we try to replace that loop

while (ISCHARALNUM (operand[len]) || operand[len] == '_')
    len++;

by whatever sdas uses to parse the symbol name? It feels inelegant to have the handling of volatile depend on the spelling of identifiers (and if a user runs notices it, we'll probably get a bug report).

[Christopher Bazley]

Hi @spth, I agree that it is inelegant but I don't understand the reason for keeping this bug open, given that the code that you want improved has been present in one form or another since r2842 (Fri, 22 Aug 2003).

If you think it's liable to be reported as a bug then you could report it as a bug or feature request with its own instructions to reproduce the issue.

Is there something I missed? Did I cause a regression by fixing this bug?

[Philipp Klaus Krause]

I've made that into a new feature request: [feature-requests:#1042].

[Philipp Klaus Krause]

Looks ok to me.