RFR(M): 8020775: PPC64 (part 12): posix signal printing

[Vladimir Kozlov]

Hi, Goetz

On 8/14/13 6:02 AM, Lindenmaier, Goetz wrote:
> Hi,
>
>> I think it is good cleanup. Thank you, Goetz.
> Thanks! Merits go to Thomas Stuefe who initially redesigned this.
> I added him as contributor.
>
> http://cr.openjdk.java.net/~goetz/webrevs/8020775-print_sig-2/
>
> In general: The 'describe' functions compute the string containing
> the desired information.  The 'print' functions decide how to
> publish this information.  It's just a separation of concern.  We even
> test them separately.
> We have a row of additional signal printing routines that are not
> 'short'.  We use them to write much more information into the hs_err
> file.  I did not contribute them, as I assume you will not like it if the
> hs_err file get's bigger.
> In addition I added support for real time signals, and a workaround for redundant
> SIGIOT.
>
>> Could you align elements/names in tables in get_signal_name() and
>> get_signal_code_description() to make them aligned with each other and
>> with #ifdefs?
> Done, see webrev.  It looks better for most of the tables, but not
> for info[] in get_signal_name. I think it does not help there, I would prefer
> the layout without all the white space.

I like this new layout even in info[], thanks.

>
>> It looks like all unixes (including bsd) have NSIG value defined (I
>> could be wrong). you can use it directly instead of MAX_SIGNAL_NUMBER.
> There are some good reasons not to use NSIG for this constant, but
> they are in the code generating more verbose output, which I did not contribute.
> So I removed it.  (E.g., sometimes it's a function and thus can not be used
> as array size.)

Okay.

>
>> describe_signal_set_short() method is bogus. Why you overwrite buf[0]
>> with "0" or "1" and the rest of buffer with 0 each time? Should it be
>> And why you need separate describe_signal_set_short() method? And why
>> you need *10 for buffer size
> Oh, you are right, fixed.  I had to change this, as we are using fixBufferStream for

I still do not get your code in describe_signal_set_short(). Why not do 
it like next? Note, I changed '<' to '<=' to print 32 and 31:

const char* os::Posix::describe_signal_set_short(const sigset_t* set, 
char* buffer, size_t buf_size) {
   assert(buf_size = (NUM_IMPORTANT_SIGS + 1), "wrong buffer size");
   // Note: for shortness, just print out the first 32. That should
   // cover most of the useful ones, apart from realtime signals.
   for (int sig = 1; sig <= NUM_IMPORTANT_SIGS; sig++) {
     const int rc = sigismember(set, sig);
     if (rc == -1 && errno == EINVAL) {
       buffer[sig-1] = '?';
     } else {
       buffer[sig-1] = rc == 0 ? '0' : '1';
     }
   }
   buffer[NUM_IMPORTANT_SIGS] = 0;
   return buffer;
}

> printing here.  We implemented fixBufferStream in ostream.hpp extending
> outputStream to print to a given, fixed buffer.
> I didn't want to contribute that because it's used only here in the port.
>> I would prefer to see the code similar to
>> describe_sa_flags() with list of signals instead of "01". Yes, it would
>> be different from current code but it would much more useful to have
>> signal names.
> It's the short routine, replacing an int printed in a line.  Signal names
> would be far too long here.  I can contribute

Okay, yes it would be too long.

>    STEP(225, "(printing signal handlers)" )
>       if (_verbose) {
> #ifdef _WIN32
>         os::print_signal_handlers(st, buf, sizeof(buf));
> #else
>         os::Posix::print_current_signal_handlers(st);
> #endif
>         st->cr();
>       }
> which would do what you have in mind here, I think.
>
>> In describe_sa_flags():
>> size_t remaining = size-1; // leave space for /0
> I think this is correct.  Below we test accordingly.

The code in the loop in describe_sa_flags() is incorrect because 
strlen(p) gives size of all stored sa names. It should be 
strlen(flaginfo[idx].s). I also don't see why it is while() loop and not 
simple for().

>
>> In get_signal_code_description(): Add break:
> Fixed.
>> Put on different lines:
>> +    out->s_name = out->s_desc = "unknown";
> Fixed.
>
>> Why not use s_name instead of s_code to match out->s_name?
> I renamed this a bit, I hope this makes it better.

Good.

>
> If you think it's useful for OpenJDK, I can contribute the fixBufferStream
> and the more verbose printing methods.

Not now :)

thanks,
Vladimir

>
> Best regards,
>    Goetz.
>
> On 7/25/13 4:11 PM, Lindenmaier, Goetz wrote:
>> Hi,
>>
>> we'd like to contribute our posix signal printing.
>> We implemented some routines to print signal and sa_flags information
>> in the os/posix files, and call them from
>> os::print_siginfo and print_signal_handler() in the various unix
>> variant directories.
>> The output is a bit more verbose than the existing version.
>>
>> We contribute this here, as our aix code uses this too.
>>
>> Please review this and test it if you think we should add this.
>> We'd appreciate it.
>> http://cr.openjdk.java.net/~goetz/webrevs/8020775-print_sig/
>>
>> Thanks and best regards,
>>     Goetz.
>>