Thank you for reviews. Pushed.
-Dmitry
On 3/2/19 2:28 AM, Chris Plummer wrote:
+1
thanks,
Chris
On 3/1/19 2:56 PM, David Holmes wrote:
Looks good.
Thanks,
David
On 2/03/2019 7:46 am, Dmitry Chuyko wrote:
Ok. typedef part removed according to review comments (not related
to fix).
Updated webrev: http://cr.openjdk.java.net/~dchuyko/8214854/webrev.01/
-Dmitry
On 3/1/19 10:07 PM, Chris Plummer wrote:
On 2/28/19 11:12 PM, Dmitry Chuyko wrote:
On 3/1/19 6:36 AM, Chris Plummer wrote:
On 2/28/19 5:07 PM, David Holmes wrote:
Hi Dmitry,
On 1/03/2019 6:15 am, Dmitry Chuyko wrote:
Hello,
Please review a small fix for GCC 8.x warning in
log_messages.c. Buffer sizes for parts of timestamp string can
be adjusted to not exceed maximum buffer size when combined,
while being able to hold necessary information.
webrev: http://cr.openjdk.java.net/~dchuyko/8214854/webrev.00/
bug: https://bugs.openjdk.java.net/browse/JDK-8214854
testing: after the fix GCC 8 compiles working OpenJDK on Linux.
Dev-submit job started.
This looks good.
One query:
51 typedef char timestamp_buffer[TIMESTAMP_SIZE];
Is this needed so that the size of the buffer is known by gcc
inside get_time_stamp? (Generally I prefer to see explicitly
when an array is being stack allocated, not have it hidden
behind a typedef.)
This also caught my attention and then got me thinking on what
basis is gcc producing the warning in the first place. The
warning is:
src/jdk.jdwp.agent/share/native/libjdwp/log_messages.c:75:24:
error: '%.3d' directive output may be truncated writing between 3
and 11 bytes into a region of size between 0 and 80
[-Werror=format-truncation=]
"%s.%.3d %s", timestamp_prefix,
^~~~
I think it deduced this by seeing that the call to
get_time_stamp() is passing in a char[80] for the tbuf argument.
It then also sees that the argument for the first %s is also a
char[80] array, so is pointing out that this first %s might fill
(or nearly fill) tbuf, not leaving room for the %.3d argument. If
this is the case, there is no need for the timestamp_buffer
typedef. The real fix was making timestamp_date_time and
timestamp_timezone smaller so gcc doesn't think they can overflow
tbuf.
Chris
Current signature of helper function is get_time_stamp(char *tbuf,
size_t ltbuf). If we drop typedef change, this function either
logically needs ltbuf parameter or it internally assumes it to be
somewhat (TIMESTAMP_SIZE+1).
Hi Dmitry,
Ok, but that's unrelated to the compiler warning and not needed for
the fix. Using a typedef hides the fact that we are dealing with a
char* so makes the code less readable. I actually prefer the way it
was with a char* and size_t arguments. My next preference would be:
get_time_stamp(char tbuf[MAXLEN_TIMESTAMP+1])
Although I think it's also a bit odd. I don't recall seeing array
sizes in function prototypes before.
thanks,
Chris
In first case we in general should care about arbitrary ltbuf
size. Like 5 or 2. In second case there are no static checks/hints
that we don't pass tbuf of diferent size. So I preferred to have
this information in the function signature and to use known
lengths of parts: get_time_stamp(timestamp_buffer tbuf).
-Dmitry
Thanks,
David
-Dmitry