Page MenuHomeFreeBSD

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

Authored by markj on Jul 31 2015, 1:21 AM.
Tags
None
Referenced Files
F101908390: D3255.id7658.diff
Tue, Nov 5, 11:55 AM
Unknown Object (File)
Mon, Oct 28, 5:47 AM
Unknown Object (File)
Sep 20 2024, 8:19 PM
Unknown Object (File)
Sep 17 2024, 9:28 PM
Unknown Object (File)
Sep 17 2024, 9:20 PM
Unknown Object (File)
Sep 17 2024, 9:18 AM
Unknown Object (File)
Sep 12 2024, 6:06 AM
Unknown Object (File)
Sep 1 2024, 7:20 AM
Subscribers

Details

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 Not Applicable
Unit
Tests Not Applicable

Event Timeline

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.
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 :(.

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?

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.

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 edited edge metadata.
  • Address some review comments:
kib edited edge metadata.

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 ?

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

markj edited edge metadata.
  • 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
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 edited edge metadata.
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 edited edge metadata.

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!

This revision was automatically updated to reflect the committed changes.