On Mon, Jul 22, 2013 at 11:20 AM, Alexander Monakov <amona...@ispras.ru> wrote: > On Mon, 22 Jul 2013, Ian Lance Taylor wrote: >> Thanks for noticing the problem. This patch isn't enough by itself. >> The code has to protect itself against the list changing in >> mid-stream. See dwarf_fileline in dwarf.c. > > Thank you. Here's the updated patch, however I have to admit it's not > entirely clear to me what __sync_bool_compare_and_swap should achieve. Is it > only to ensure that we do not use a partially updated pointer (which shouldn't > happen with a naturally aligned pointer on hardware that updates cache lines > atomically)?
We not only need to get a valid pointer, we need to ensure when thread T1 creates the pointer and thread T2 loads the pointer, thread T2 sees all the global stores that thread T1 made before T1 set the pointer. That is, we need an acquire-load. The hope here is that any sane implementation of __sync_bool_compare_and_swap would ensure sequential consistency between T1 and T2, implying an acquire-load. On x86 every load is an acquire-load, but that is not true on other processors. If we added some configure tests, or didn't worry about letting people use this library with older versions of GCC, we could use __atomic_load_n (pp, __ATOMIC_ACQUIRE) here, and use something like __atomic_compare_exchange_n (pp, NULL, edata, true, __ATOMIC_RELEASE, __ATOMIC_RELAXED) in elf_add_syminfo_data. I don't think it would make any difference on x86, though. > 2013-07-22 Alexander Monakov <amona...@ispras.ru> > > * elf.c (elf_syminfo): Loop over the elf_syminfo_data chain. > + for (edata = (struct elf_syminfo_data *) state->syminfo_data; > + edata; Please explicitly write edata != NULL. This is OK with that change. Thanks. Ian
