This is to make it easier to implement stack saving for running threads
without duplicating code.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| 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? |
Comment Actions
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. |
Comment Actions
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 ? |
Comment Actions
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()?
Comment Actions
- Remove amd64_frame's f_arg0 and restore some upstream DTrace code that
- Use the "=g" constraint in stack_save().
Comment Actions
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 |
Comment Actions
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!