Please do not reply to this email. Use the web interface provided at: http://bugs.ecos.sourceware.org/show_bug.cgi?id=1001607
--- Comment #17 from Jonathan Larmour <[email protected]> 2012-08-07 06:57:07 BST --- More comments.... 7) There's a few more things from cortexm_stub.h which you put in fpv4_sp_d16.h which I don't think need to be: HAL_STUB_REGISTERS_SIZE, PS_N/Z/C/V, the definition of target_register_t and TARGET_HAS_LARGE_REGISTERS. These definitions will be the same everywhere in Cortex-M so can just live once in cortexm_stub.h. 8) In fpv4_sp_d16.h, hal_cortexm_fpu_enable/disable are still extern inline, which as I mentioned in comment #9 can be problematic. You are slightly at risk of link errors depending on the compiler optimisations. So these should be HAL_CORTEXM_FPU_ENABLE/DISABLE preprocessor macros instead. 9) CYGARC_CORTEXM_FPU_EXC_AUTOSAVE defaults to off. 10) A bit like my previous comment about lining up things in CDL, lining up some things here can be clearer too. For example: # define HAL_SAVEDREG_MAN_EXCEPTION_FPU_N 16 # define HAL_SAVEDREG_AUTO_EXCEPTION_FPU_N 16 # define HAL_SAVEDREG_AUTO_FRAME_SIZE (8*4 + 16*4 + 4 + 4) // HAL_SavedRegisters entries for floating point registers // see hal_arch.h for HAL_SavedRegisters definition. # define HAL_SAVEDREG_AUTO_FPU_EXCEPTION_S \ cyg_uint32 s_auto[HAL_SAVEDREG_AUTO_EXCEPTION_FPU_N]; \ cyg_uint32 fpscr; \ cyg_uint32 align; is a little better as: # define HAL_SAVEDREG_MAN_EXCEPTION_FPU_N 16 # define HAL_SAVEDREG_AUTO_EXCEPTION_FPU_N 16 # define HAL_SAVEDREG_AUTO_FRAME_SIZE (8*4 + 16*4 + 4 + 4) // HAL_SavedRegisters entries for floating point registers // see hal_arch.h for HAL_SavedRegisters definition. # define HAL_SAVEDREG_AUTO_FPU_EXCEPTION_S \ cyg_uint32 s_auto[HAL_SAVEDREG_AUTO_EXCEPTION_FPU_N]; \ cyg_uint32 fpscr; \ cyg_uint32 align; Well, personally I find it easier to read anyway. 11) hal_arch.inc: The line indentation isn't very consistent through this, including of comments. It may also be helpful to put a comment separator between the two halves (i.e. //=========================== etc.). 12) hal_fpu_unenable surely ought to be named just hal_fpu_disable ? Ditto all the other mentions of unenable. Just wondering if you're using it specifically or just temporarily forgot the word :-). Bear in mind I don't know a single word of Macedonian so I'm grateful your English is so good!. 13) In fpv4_sp_d16.h the uses of __regs_p in HAL_THREAD_INIT_FPU_REGS and _CONTEXT still need brackets for safety :). 14) In hal_arch.h, CYGNUM_HAL_STACK_CONTEXT_SIZE still needs abstracting so that different FPUs can have different sizes (as per comment #9) 15) In hal_arch.inc, hal_fpu_enable and _disable should use CYGARC_REG_FPU_CPACR_ENABLE to be consistent 16) The banner at the top of hal_arch.inc isn't right (it says vectors.S) and the description of cortexm_fpu.c could be better - I think that was just taken from vectors.S as well. 17) As mentioned in comment #9, I think all the usage fault processing in vectors.S and hal_misc.c should only be present if lazy FPU context switching is enabled. Otherwise let it default to the hal_default_exception_vsr. That's the only time we need it. 18) Before, I had mentioned about saving FP context with interrupts and exceptions. I was making two observations there, one was about interrupts (thanks for sorting that out with LSPEN/ASPEN) but the other is that, unless the user explicitly requests it, we should only save the FP state for exceptions if we're a ROM monitor or have GDB stubs. At the moment they're always saved, which I think is unnecessary for most people's production applications. So I think we need an option just to cover the hal_fpu_exc_push/pop calls in hal_default_exception_vsr. 19) reg_offset() in cortexm_stub.c still has a diag_printf. It also would be improved with some abstraction. I _think_ the following untested code would do it and then we wouldn't need two different versions at all: static int reg_offset(regnames_t reg) { int i, offset; for (i=0,offset=0; i<NUMREGS; i++) { if ( i == reg ) break; offset += REGSIZE(i); } if ( NUMREGS == i || REGSIZE(i) == 0 ) return -1; return offset; } since this is only used by the stub, a tiny amount of runtime inefficiency is fine. 20) vectors.S still has a "NOTE:p" accidental change on line 158. 21) It looks like we still may not agree about whether usage faults should have their own exception vsr or not. As I said they are only used with lazy FPU handling and at most once per thread, so this is not performance critical code. Code size might be critical though. But the biggest problem is that, as you say, at present the hal_usagefault_exception_vsr has a different prologue/epilogue to hal_default_exception_vsr. That means the user can't debug any usage faults such as illegal instructions or unaligned accesses because the stub won't be entered. Whether with a separate vsr or not, I think it is important we have a solution that still allows users to debug usage faults. I guess that if we did do this through hal_default_exception_vsr then it would mean having to test whether the FPU was enabled or not before attempting to save the context. On the other hand, exceptions are of course rare and usually not something programs rely on for normal operation (only fatal errors usually), so performance probably isn't critical either. Can you think of any other way to allow usage faults to be debuggable when using the usage fault exception and lazy FPU context switching? I'm prepared to make many of these changes as it is unfair of me to ask you to do them all when you have already done so much, but given how much time has passed since this patch, I'm not sure if you have made other changes in the meantime, so let me know if you want me to make changes, so we can avoid clashes and having to merge. Благодарам ! Jifl -- Configure bugmail: http://bugs.ecos.sourceware.org/userprefs.cgi?tab=email ------- You are receiving this mail because: ------- You are on the CC list for the bug.
