On Tue, Mar 23, 2021, 12:26 PM Alex White <alex.wh...@oarcorp.com> wrote:
> On Tue, Mar 23, 2021 at 11:46 AM Gedare Bloom <ged...@rtems.org> wrote:
> >
> > On Tue, Mar 23, 2021 at 10:31 AM Alex White <alex.wh...@oarcorp.com>
> wrote:
> > >
> > > ---
> > > .../aarch64/aarch64-exception-frame-print.c | 133 ++++++++++++++++--
> > > 1 file changed, 123 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/cpukit/score/cpu/aarch64/aarch64-exception-frame-print.c
> b/cpukit/score/cpu/aarch64/aarch64-exception-frame-print.c
> > > index 59b5d06032..a8c492b1da 100644
> > > --- a/cpukit/score/cpu/aarch64/aarch64-exception-frame-print.c
> > > +++ b/cpukit/score/cpu/aarch64/aarch64-exception-frame-print.c
> > > @@ -45,10 +45,111 @@
> > > #include <inttypes.h>
> > >
> > > #include <rtems/score/cpu.h>
> > > +#include <rtems/score/io.h>
> > > #include <rtems/bspIo.h>
> > >
> > > +typedef struct {
> > > + char *s;
> > > + size_t n;
> > > +} String_Context;
> > > +
> > > +static void _CPU_Put_char( int c, void *arg )
> > > +{
> > > + String_Context *sctx = arg;
> > > + size_t n = sctx->n;
> > > +
> > > + if (n > 0) {
> > > + char *s = sctx->s;
> > > + *s = (char) c;
> > > + sctx->s = s + 1;
> > > + sctx->n = n - 1;
> > > + }
> > > +}
> > > +
> > > +static void _CPU_Binary_sprintf(
> > > + char *s,
> > > + size_t maxlen,
> > > + uint32_t num_bits,
> > > + uint32_t value
> > > +)
> > > +{
> > > + String_Context sctx;
> > > + uint32_t mask;
> > > +
> > > + sctx.s = s;
> > > + sctx.n = maxlen;
> > > +
> > > + mask = 1 << (num_bits - 1);
> > > +
> > > + while ( mask != 0 ) {
> > > + _IO_Printf( _CPU_Put_char, &sctx, "%d", (value & mask ? 1 : 0) );
> > > + mask >>= 1;
> > > + }
> >
> > should this function default to NUL-terminate s?
> >
> > I'm not totally familiar with the usage, but usually that is the
> > expectation of an sprintf() kind of function.
>
> I decided that the function should not NULL-terminate in this case
> because it is only used in two places, and I zero-initialize the
> character buffers before both calls.
>
> I realize that NULL-termination would make the function more generally
> useful, but I only wrote it as a quick static helper for this file.
>
> Is it worth NULL-terminating to avoid a possible bug being introduced
> in the future if someone assumes that it is NULL-terminating?
>
Yeah. I've been around long enough where not null terminating it will
actually cause someone a problem. And you should never have to zero
something out if you fill it out properly. :)
> >
> > > +}
> > > +
> > > +static const char* _CPU_Exception_class_to_string( uint16_t
> exception_class )
> > > +{
> > > + switch ( exception_class ) {
> > > + case 0x01:
> > > + return "Trapped WFI or WFE instruction";
> > > + case 0x03:
> > > + return "Trapped MCR or MRC access with (coproc==0b1111)";
> > > + case 0x04:
> > > + return "Trapped MCRR or MRRC access with (coproc==0b1111)";
> > > + case 0x05:
> > > + return "Trapped MCR or MRC access with (coproc==0b1110)";
> > > + case 0x06:
> > > + return "Trapped LDC or STC access";
> > > + case 0x0c:
> > > + return "Trapped MRRC access with (coproc==0b1110)";
> > > + case 0x0e:
> > > + return "Illegal Execution state";
> > > + case 0x18:
> > > + return "Trapped MSR, MRS, or System instruction";
> > > + case 0x20:
> > > + return "Instruction Abort from a lower Exception level";
> > > + case 0x21:
> > > + return "Instruction Abort taken without a change in Exception
> level";
> > > + case 0x22:
> > > + return "PC alignment fault";
> > > + case 0x24:
> > > + return "Data Abort from a lower Exception level";
> > > + case 0x25:
> > > + return "Data Abort taken without a change in Exception level";
> > > + case 0x26:
> > > + return "SP alignment fault";
> > > + case 0x30:
> > > + return "Breakpoint exception from a lower Exception level";
> > > + case 0x31:
> > > + return "Breakpoint exception taken without a change in
> Exception level";
> > > + case 0x32:
> > > + return "Software Step exception from a lower Exception level";
> > > + case 0x33:
> > > + return "Software Step exception taken without a change in
> Exception "
> > > + "level";
> >
> > This is a case where I would say allowing the string to spill over the
> > 80c is better. I think that having a long string (say 40+ characters)
> > can justify violating this 80c limit. It would be best for someone to
> > agree with me though :)
>
> I agree that it would look better if I allowed this line to exceed the
> 80 character limit. Those darn coding conventions...
>
> Hopefully others agree to an exception here.
>
I agree. Put a comment above the switch that it is deliberately violated.
Seeing the values in makes it clear that there are categories to these
values based on the first nibble. I don't know that there's any value to
taking advantage of that but the pattern is there. Perhaps if more the
message can be common but only if the first nibble is used to decode more
detail. This is mostly an idle observation. No changes required.
> >
> > Address the NUL termination, and decide how you want to proceed with
> > the line length for string breaks. It's a gray area in the rules.
> >
> > > + case 0x34:
> > > + return "Watchpoint exception from a lower Exception level";
> > > + case 0x35:
> > > + return "Watchpoint exception taken without a change in
> Exception level";
> > > + case 0x38:
> > > + return "BKPT instruction execution in AArch32 state";
> > > + case 0x3c:
> > > + return "BRK instruction execution in AArch64 state";
> > > + default:
> > > + return "Unknown";
> > > + }
> > > +}
> > > +
> > > void _CPU_Exception_frame_print( const CPU_Exception_frame *frame )
> > > {
> > > + uint32_t ec;
> > > + uint32_t il;
> > > + uint32_t iss;
> > > + char ec_str[7] = { 0 };
> > > + char iss_str[26] = { 0 };
> > > + int i;
> > > + const uint128_t *qx;
> > > +
> > > printk(
> > > "\n"
> > > "X0 = 0x%016" PRIx64 " X17 = 0x%016" PRIx64 "\n"
> > > @@ -68,8 +169,7 @@ void _CPU_Exception_frame_print( const
> CPU_Exception_frame *frame )
> > > "X14 = 0x%016" PRIx64 " SP = 0x%016" PRIxPTR "\n"
> > > "X15 = 0x%016" PRIx64 " PC = 0x%016" PRIxPTR "\n"
> > > "X16 = 0x%016" PRIx64 " DAIF = 0x%016" PRIx64 "\n"
> > > - "VEC = 0x%016" PRIxPTR " CPSR = 0x%016" PRIx64 "\n"
> > > - "ESR = 0x%016" PRIx64 " FAR = 0x%016" PRIx64 "\n",
> > > + "VEC = 0x%016" PRIxPTR " CPSR = 0x%016" PRIx64 "\n",
> > > frame->register_x0, frame->register_x17,
> > > frame->register_x1, frame->register_x18,
> > > frame->register_x2, frame->register_x19,
> > > @@ -83,23 +183,36 @@ void _CPU_Exception_frame_print( const
> CPU_Exception_frame *frame )
> > > frame->register_x10, frame->register_x27,
> > > frame->register_x11, frame->register_x28,
> > > frame->register_x12, frame->register_fp,
> > > - frame->register_x13, (intptr_t)frame->register_lr,
> > > - frame->register_x14, (intptr_t)frame->register_sp,
> > > - frame->register_x15, (intptr_t)frame->register_pc,
> > > + frame->register_x13, (intptr_t) frame->register_lr,
> > > + frame->register_x14, (intptr_t) frame->register_sp,
> > > + frame->register_x15, (intptr_t) frame->register_pc,
> > > frame->register_x16, frame->register_daif,
> > > - (intptr_t) frame->vector, frame->register_cpsr,
> > > - frame->register_syndrome, frame->register_fault_address
> > > + (intptr_t) frame->vector, frame->register_cpsr
> > > );
> > >
> > > - const uint128_t *qx = &frame->register_q0;
> > > - int i;
> > > + ec = frame->register_syndrome >> 26 & 0x3f;
> > > + il = frame->register_syndrome >> 25 & 0x1;
> > > + iss = frame->register_syndrome & 0x1ffffff;
> > > +
> > > + _CPU_Binary_sprintf( ec_str, sizeof( ec_str ), sizeof( ec_str ) -
> 1, ec );
> > > + _CPU_Binary_sprintf( iss_str, sizeof( iss_str ), sizeof( iss_str )
> - 1, iss );
> > > +
> > > + printk(
> > > + "ESR = EC: 0b%s" " IL: 0b%d" " ISS: 0b%s" "\n"
> > > + " %s\n",
> > > + ec_str, il, iss_str, _CPU_Exception_class_to_string( ec )
> > > + );
> > > +
> > > + printk( "FAR = 0x%016" PRIx64 "\n", frame->register_fault_address
> );
> > > +
> > > + qx = &frame->register_q0;
> > >
> > > printk(
> > > "FPCR = 0x%016" PRIx64 " FPSR = 0x%016" PRIx64 "\n",
> > > frame->register_fpcr, frame->register_fpsr
> > > );
> > >
> > > - for ( i = 0; i < 32; ++i ) {
> > > + for ( i = 0 ; i < 32 ; ++i ) {
> > > uint64_t low = (uint64_t) qx[i];
> > > uint64_t high = (uint64_t) (qx[i] >> 32);
> > >
> > > --
> > > 2.27.0
> > >
> > > _______________________________________________
> > > devel mailing list
> > > devel@rtems.org
> > > http://lists.rtems.org/mailman/listinfo/devel
> > _______________________________________________
> > devel mailing list
> > devel@rtems.org
> > http://lists.rtems.org/mailman/listinfo/devel
> _______________________________________________
> devel mailing list
> devel@rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
_______________________________________________
devel mailing list
devel@rtems.org
http://lists.rtems.org/mailman/listinfo/devel
