stack(9): merge amd64 and i386 implementations under x86

[markj]

Summary

This is to make it easier to implement stack saving for running threads
without duplicating code.

[kib]

sys/x86/include/stack.h

36 ↗

(On Diff #7536)

This definition is wrong when compiled on i386. You probably should use uint64_t for f_retaddr/f_arg0, and I am not sure what to do with the pointer.

Why do you need both defs ? I would agree with usefulness of having i386 definition on amd64, but amd64 on i386 is arguably useless.

39 ↗

(On Diff #7536)

Mere presence of the f_arg0 is wrong on amd64. I assume it is not used.

sys/x86/x86/stack_machdep.c

99 ↗

(On Diff #7536)

"r" is too restrictive constraint there. "g" might work better, except clang apparently generates code for "g" which always goes through the memory :(.

[markj]

sys/x86/include/stack.h

36 ↗

(On Diff #7536)

I don't have any particular need for amd64_frame on i386. I'll #ifdef it out.

The problem you pointed out also exists for i386_frame on amd64. I think it might be cleanest to fix this by defining i386_frame twice: once for native usage and once for amd64, with fields of type uint32_t for the latter.

39 ↗

(On Diff #7536)

As far as I can tell, no. This definition was presumably just copied from i386 when it was first added to DDB.

In fact, the presence of f_arg0 has caused bugs in DTrace code which uses sizeof(struct amd64_frame) in some pointer arithmetic: Solaris defines struct amd64_frame as well, but without the f_arg0 field. I can remove it in a separate change, but will need to do a sweep to fix up code which depends on the size.

sys/x86/x86/stack_machdep.c

99 ↗

(On Diff #7536)

I'm not sure what you mean - it seems like the compiler should always be able to satisfy this constraint?

[kib]

sys/x86/include/stack.h

36 ↗

(On Diff #7536)

Fine with me.

39 ↗

(On Diff #7536)

Ok.

sys/x86/x86/stack_machdep.c

99 ↗

(On Diff #7536)

g gives more freedom to the compiler.

[jhb]

I agree to the suggestions to drop f_arg0 from the amd64 frame. If machine/frame.h isn't actually used in userland you could actually go more radical and do something like this:

  struct x86_frame {
      struct x86_frame *f_frame;
      register_t  f_retaddr;
#ifdef __i386__
      register_t  f_arg0;
#endif
  }

However, you can't quite get away with that if there are things that really expect to see 'i386_frame' and 'amd64_frame' as types.

sys/x86/include/stack.h

48 ↗

(On Diff #7536)

This seems to be from your other changes?

sys/x86/x86/stack_machdep.c

44 ↗

(On Diff #7536)

Maybe call it TF_PC() instead of TF_IP()? PC is the typical MI name.

[markj]

• Address some review comments:

[kib]

The types note is minor, handle it as you consider appropriate.

I am fine with the patch.

sys/x86/include/stack.h

39 ↗

(On Diff #7658)

Should all types be unsigned ?

[jhb]

I would still suggest removing f_arg0 from amd64_frame.

Also, it seems you are still using 'r' instead of 'g' constraint for reading 'fp' in stack_save()?

[markj]

• Remove amd64_frame's f_arg0 and restore some upstream DTrace code that
• Use the "=g" constraint in stack_save().

[markj]

In D3255#66838, @jhb wrote:

I would still suggest removing f_arg0 from amd64_frame.

Also, it seems you are still using 'r' instead of 'g' constraint for reading 'fp' in stack_save()?

I was going to remove f_arg0 in a different change, but the fallout is minimal (just had to revert an old DTrace change to some code which fetches function arguments off the stack). I'll still commit it as a separate change, I think.

The constraint is updated.

sys/x86/x86/stack_machdep.c

99 ↗

(On Diff #7742)

I changed the code to use "=g" in both cases. clang 3.6.1 appears to emit the same instructions in both cases, FWIW: http://pastebin.com/raw.php?i=QhQb7YF9

[kib]

In D3255#67454, @markj wrote:

I was going to remove f_arg0 in a different change, but the fallout is minimal (just had to revert an old DTrace change to some code which fetches function arguments off the stack). I'll still commit it as a separate change, I think.

I agree.

[jhb]

Ah, sorry, I hadn't realized you were going to do f_arg0 separately. I think it is fine to commit separately as well. Thanks!