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