Re: [PATCH] Fix PR51513, switch statement with default case containing __builtin_unreachable leads to wild branch

[Peter Bergner <bergner at vnet dot ibm dot com>]

Bah, fixing up my return address.


On 4/13/17 3:14 AM, Richard Biener wrote:
> To recap the situation (from what I can deciper out of the ppc asm
> and the expand RTL) we seem to expand to
> 
>   if (cond > 2)
>     __builtin_unreachable (); // jumps to the jump table data(?)
>   goto *tbl[cond];
> 
> now I do not remember the reason why we keep __builtin_unreachable ()
> at the RTL level -- on GIMPLE we keep it to be able to extract
> range information from the controlling condition.

As Jakub said, we do remove the blocks.  But we don't remove the branch
to those blocks.  That is true of whether the __builtin_unreachable() is
in the default case statement or in a normal case statement.  That is
what leads to the wild branch issue.


> Your patch comments suggest that handling of gaps is similarly
> improved but the testcase doesn't contain any gaps.

I didn't change the way the jump table gaps are handled.  They have
always been redirected to the "default label"... although the patch
doesn't use default_label anymore, it uses gap_label, as resetting
default_label to fallback_label when default_label is NULL was
what was stopping us from removing the range check.

What is changed, is that now we also handle case labels that point to
blocks with __builtin_unreachable() and those are also redirected to
the new gap_label.

I did not include a test case with an unreachable case statement,
because unlike the unreachable default case test case, there is no
compare/branch I can verify has been removed.  However, looking
closer, it seems the unpatched compiler leaves the unreachable
block as well as it's label in the jump table, so I should be able
to create an executable test case that uses a switch condition
that targets the unreachable case and see if we SEGV/SIGILL or not.
I cannot do that for the unreachable default case, since the range
check we remove is what saves us from accessing the jump table
with an index that is outside of the table.



> So I wonder why we don't simply apply the "transform" at the GIMPLE
> level, for example in the kitchen-sink pass_fold_builtins.  That is
> remove all __builtin_unreachable () labels and if the default label
> is amongst them make the first one the new default (or maybe the
> last one dependent on which would shrink jump table size most).

Well if the default case is unreachable and we compute a "new"
default, then we won't be able to eliminate the range check
(ie, compare/branch to default).  So isn't it performance wise
better to eliminate the range check, rather than choosing a
new default?  If you think we shouldn't care about that, then
I'd vote for choosing the first/last label with the higher
edge probability rather than trying to shrink the table.




> VRP2 removes the if because we put the range computed by VRP1
> on the SSA name used in the test (we have special code handling
> unreachable paths there).  For your testcase with the switch
> as cond_2 is not used after the switch we do not compute any
> range for it, otherwise we'd likely eliminate the default
> case as well.  For the small testcase above fold-all-builtins
> handles the removal as well if VRP is not run, so extending
> that to handle switch stmts sounds reasonable (see
> optimize_unreachable).  There's even a TODO in this function.

If we updated optimize_unreachable() to remove unreachable
default cases, would we still be able to disambiguate between
a switch statement written with no default case and one where
the default case was explicitly shown to be unreachable?
Maybe the default_label would be NULL for the unreachable
case and non-NULL in the other case?  If so, we'd still need
my change that doesn't set default_label to fallback_label
and instead uses the new var gap_label.

Peter