bug#28211: Stack marking issue in multi-threaded code

[ludo@gnu.org (Ludovic Courtès)]

Hi Mark,

Mark H Weaver <mhw@netris.org> skribis:

> ludo@gnu.org (Ludovic Courtès) writes:

[...]

>> I’m thinking we could perhaps add a compiler barrier before ‘vp->fp = new_fp’
>> statements, but in practice it’s not necessary here (x86_64, gcc 7).
>>
>> Thoughts?

I just pushed the patch as 23af45e248e8e2bec99c712842bf24d6661abbe2.

> Indeed, we need something to ensure that the compiler won't reorder
> these writes.  My recommendation would be to use the 'volatile'
> qualifier in the declarations of both the 'fp' field of 'struct scm_vm'
> and the stack element modified by SCM_FRAME_SET_DYNAMIC_LINK.
>
> Maybe something like this:
>
> diff --git a/libguile/frames.h b/libguile/frames.h
> index ef2db3df5..8554f886b 100644
> --- a/libguile/frames.h
> +++ b/libguile/frames.h
> @@ -89,6 +89,7 @@
>  union scm_vm_stack_element
>  {
>    scm_t_uintptr as_uint;
> +  volatile scm_t_uintptr as_volatile_uint;

[...]

> +#define SCM_FRAME_SET_DYNAMIC_LINK(fp, dl) ((fp)[1].as_volatile_uint = ((dl) - (fp)))

I’m not sure this is exactly what we need.

What I had in mind, to make sure the vp->fp assignment really happens
after the SCM_FRAME_SET_DYNAMIC_LINK, was to do something like this:

  SCM_FRAME_SET_RETURN_ADDRESS (new_fp, …);
  SCM_FRAME_SET_DYNAMIC_LINK (new_fp, …);

  asm volatile ("" : : : "memory");

  vp->fp = new_fp;

I think that more accurately reflects what we want, WDYT?

It’s GCC-specific though (I don’t think Clang implements ‘asm’ in a
compatible way, does it?) and I suppose we’d have to ignore the non-GCC
case.

Thoughts?

Ludo’.