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

Authored by markj on Jul 31 2015, 1:21 AM.

Details

Reviewers

kib
jhb
gnn

Commits

rS287644: Remove the arg0 field from struct amd64_frame. Its existence was a bug,
rS287643: Merge stack(9) implementations for i386 and amd64 under x86/.

Summary

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

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 2
Build 2: arc lint + arc unit

Event Timeline

markj updated this revision to Diff 7536.Jul 31 2015, 1:21 AM

markj retitled this revision from to stack(9): merge amd64 and i386 implementations under x86.

markj edited the test plan for this revision. (Show Details)

markj updated this object.

Herald added a subscriber: imp. · View Herald TranscriptJul 31 2015, 1:21 AM

markj added reviewers: kib, jhb.Jul 31 2015, 1:25 AM

kib added inline comments.Jul 31 2015, 3:31 AM

sys/x86/include/stack.h
45	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.
47–48	Mere presence of the f_arg0 is wrong on amd64. I assume it is not used.
sys/x86/x86/stack_machdep.c
99	"r" is too restrictive constraint there. "g" might work better, except clang apparently generates code for "g" which always goes through the memory :(.

markj added inline comments.Aug 1 2015, 12:28 AM

sys/x86/include/stack.h
45	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.
47–48	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	I'm not sure what you mean - it seems like the compiler should always be able to satisfy this constraint?

kib added inline comments.Aug 1 2015, 5:27 AM

sys/x86/include/stack.h
45	Fine with me.
47–48	Ok.
sys/x86/x86/stack_machdep.c
99	g gives more freedom to the compiler.

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
56	This seems to be from your other changes?
sys/x86/x86/stack_machdep.c
44	Maybe call it TF_PC() instead of TF_IP()? PC is the typical MI name.

Address some review comments:

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

I am fine with the patch.

sys/x86/include/stack.h
39	Should all types be unsigned ?

This revision is now accepted and ready to land.Aug 5 2015, 4:49 AM

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()?

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

This revision now requires review to proceed.Aug 7 2015, 12:42 AM

Herald added a reviewer: gnn. · View Herald TranscriptAug 7 2015, 12:42 AM

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	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

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.