Re: [PATCH v3] Include XSTATE note in x86 core dumps
Hi, First of all, I'm sorry for not replying for this long. I've got distracted with other things, and I've completely forgotten about this. On Tue, 2019-07-16 at 05:23 +1000, matthew green wrote: > > Introduce a simple COREDUMP_MACHDEP_LWP_NOTES logic to provide machdep > > API for injecting per-LWP notes into coredumps, and use it to append > > PT_GETXSTATE note. > > this macro is pretty gross. these are the problems i see: > > - assumes 'int error' This goes away if we're removing error handling. > - assumes 'struct lwp *l = curlwp' > - assumes 'name' is something > - assumes 'ns' is something Made into parameters. > - returns from macro Resolved as you suggested -- we're silently skipping this entry if it fails. I'm going to resend the whole series as v4 now. -- Best regards, Michał Górny signature.asc Description: This is a digitally signed message part
Re: [PATCH v3] Include XSTATE note in x86 core dumps
On 16.07.2019 15:34, Michał Górny wrote: > On Tue, 2019-07-16 at 05:23 +1000, matthew green wrote: >>> Introduce a simple COREDUMP_MACHDEP_LWP_NOTES logic to provide machdep >>> API for injecting per-LWP notes into coredumps, and use it to append >>> PT_GETXSTATE note. >> >> this macro is pretty gross. these are the problems i see: >> >> - assumes 'int error' >> - assumes 'struct lwp *l = curlwp' >> - assumes 'name' is something >> - assumes 'ns' is something >> > > It also relies on ELFSIZE being defined in unit using the macro. Should > I make that into a parameter as well? > Something I would like to see as decided and not introduced as unnoticed: 1. Whether we want to ship with FPREGS in core(5) notes once we will generate the same data into XSTATE structure. 2. The same for i386, but what to do with XMMREGS? I think it makes sense to generate FPREGS into core(5) files as these fields are negligible in size compared to memory segments... and keeping some symmetry with alive process debugging where FPREGS/XMMREGS will still work; however it would be inconsistent with the lack of XMMREGS for i386. Solutions: 1. Generate XMMREGS notes for i386 2. Stop generating FPREGS notes for i386 and amd64 3. Assume that this is kind of legacy baggage and just add XSTATE next to what we get now. signature.asc Description: OpenPGP digital signature
Re: [PATCH v3] Include XSTATE note in x86 core dumps
On Tue, 2019-07-16 at 05:23 +1000, matthew green wrote: > > Introduce a simple COREDUMP_MACHDEP_LWP_NOTES logic to provide machdep > > API for injecting per-LWP notes into coredumps, and use it to append > > PT_GETXSTATE note. > > this macro is pretty gross. these are the problems i see: > > - assumes 'int error' > - assumes 'struct lwp *l = curlwp' > - assumes 'name' is something > - assumes 'ns' is something > It also relies on ELFSIZE being defined in unit using the macro. Should I make that into a parameter as well? -- Best regards, Michał Górny signature.asc Description: This is a digitally signed message part
Re: [PATCH v3] Include XSTATE note in x86 core dumps
On Tue, 2019-07-16 at 05:23 +1000, matthew green wrote: > > Introduce a simple COREDUMP_MACHDEP_LWP_NOTES logic to provide machdep > > API for injecting per-LWP notes into coredumps, and use it to append > > PT_GETXSTATE note. > > this macro is pretty gross. these are the problems i see: > > - assumes 'int error' > - assumes 'struct lwp *l = curlwp' > - assumes 'name' is something > - assumes 'ns' is something > - returns from macro > > all of those should be resolved. 4 are simple, they can be easy > inputs. the return issue is more difficult, and perhaps should > be revisited (ie, perhaps just skip over this entry instead of > assuming return is right.) Will do. I have mixed feelings about ignoring errors but I guess it won't hurt that much. > > > Since the XSTATE block uses the same format on i386 and amd64, the code > > does not have to conditionalize between 32-bit and 64-bit ELF format > > on that. However, it does need to distinguish between 32-bit and 64-bit > > PT_* values. In order to do that, it reuses PT32_* constant already > > present for ptrace(), and adds a matching PT64_GETXSTATE to satisfy > > the cpp logic. > > i don't understand why the need to include netbsd32 headers or > define these separately. > > if the layout is the same, what's the benefit? we already know > to assume 32 or 64 bit from the ELF core header, right? > That's because PT_GETXSTATE constant has different value on amd64 and i386. We need to use appropriate value depending on whether the running program is 64- or 32-bit. -- Best regards, Michał Górny signature.asc Description: This is a digitally signed message part
re: [PATCH v3] Include XSTATE note in x86 core dumps
> Introduce a simple COREDUMP_MACHDEP_LWP_NOTES logic to provide machdep > API for injecting per-LWP notes into coredumps, and use it to append > PT_GETXSTATE note. this macro is pretty gross. these are the problems i see: - assumes 'int error' - assumes 'struct lwp *l = curlwp' - assumes 'name' is something - assumes 'ns' is something - returns from macro all of those should be resolved. 4 are simple, they can be easy inputs. the return issue is more difficult, and perhaps should be revisited (ie, perhaps just skip over this entry instead of assuming return is right.) > Since the XSTATE block uses the same format on i386 and amd64, the code > does not have to conditionalize between 32-bit and 64-bit ELF format > on that. However, it does need to distinguish between 32-bit and 64-bit > PT_* values. In order to do that, it reuses PT32_* constant already > present for ptrace(), and adds a matching PT64_GETXSTATE to satisfy > the cpp logic. i don't understand why the need to include netbsd32 headers or define these separately. if the layout is the same, what's the benefit? we already know to assume 32 or 64 bit from the ELF core header, right? .mrg.
