On Wed, Feb 20, 2019 at 6:53 PM Ian Lance Taylor <i...@golang.org> wrote: > > The underlying cause of PR 89170 is a bug that appears to have existed > since 2001, when the function decode_addr_const was changed to call > output_constant_def. The problem is that output_constant_def calls > compare_constant, and when compare_constant sees an ADDR_EXPR, it > calls decode_addr_const. So it is possible for output_constant_def to > call itself recursively while adding a value to the hash table. This > only happens if there are already some constants in the hash table > with the same hash code, because that is the only case in which > compare_constant can call decode_addr_constant. This works fine if > the value whose address is taken is already in the hash table. And it > works fine if the address is not in the hash table, but adding the > address does not cause the hash table to grow. > > But if we call output_constant_def with a constant that includes an > ADDR_EXPR, and if there is already a constant in the hash table with > the same hash code, and if decode_addr_constant calls > output_constant_def recursively with a constant that is not already in > the hash table, and if adding that constant causes the hash table to > grow, then the outer call to output_constant_def will wind up looking > at the wrong hash bucket. The effect is that output_constant_def will > return an rtx for a different constant. > > This unlikely sequence of events actually happened building libgo's > net/http test on PPC64, causing a miscompilation leading to a test > failure filed as PR 89170. > > I plan to commit this patch to fix the problem. I didn't add a test > case since the sequence of events is so hard to recreate. I added a > check that will detect any future recursive insertion into the hash > table. > > Bootstrapped and tested on x86_64-pc-linux-gnu. Since I haven't > looked at this part of the code in many years, I'll wait a bit to see > if anybody has any comments on the patch.
Looks reasonable. Factoring the common code in output_constant_def and tree_output_constant_def might be worth it. Richard. > Ian > > > 2019-02-20 Ian Lance Taylor <i...@golang.org> > > PR go/89170 > * varasm.c (decode_addr_const): Call lookup_constant_def rather > than output_constant_def. > (const_desc_htab_inserting): New static variable. > (output_constant_def): Call output_addressed_constants. Check and > set const_desc_htab_inserting. > (tree_output_constant_def): Likewise.
