RFR: JDK-8013736: [launcher] cleanup code for correctness

[Kumar Srinivasan]

On 5/6/2013 7:38 PM, Martin Buchholz wrote:
> This looks good.
>
> There's one check I found confusing.
>
> 175     if (alen <= 0 || alen > INT_MAX / sizeof(char *)) {
>
> Not that it matters much, but I'm not sure exactly what you're trying
> to check here.
> If you're trying to check that argc+2 doesn't overflow INT_MAX, I
> would do that directly in the original argc check.

Making sure alen won't overflow, I will look into your suggestion.

Kumar

> If you're trying to check that alen won't overflow, I would check something like
> (argc+2) < SIZE_MAX/sizeof(const char *)
>
> On 5/6/13, Kumar Srinivasan <kumar.x.srinivasan at oracle.com> wrote:
>> Here is the modified webrev:
>> http://cr.openjdk.java.net/~ksrini/8013736/webrev.1/
>>
>> delta webrev to the last webrev:
>> http://cr.openjdk.java.net/~ksrini/8013736/webrev.1/webrev.delta/index.html
>>
>> I added the do { ..... } while (0) pattern to all the launcher's macro
>> defs,
>> I also addressed Alan's comments.
>>
>> I know that the native unpacker uses macros which will need this pattern
>> I will fix that separately.
>>
>> Alan, do you want me to search and file bugs on other jdk components ?
>>
>> Thanks
>> Kumar
>>
>>
>>
>>> Oops.  Of course, that shouldn't have the trailing semicolon
>>>
>>> #define NULL_CHECK_OR_RETURN(ncor_pointer_to_check,     \
>>>       ncor_failure_return_value) \
>>>    do {                                  \
>>>      if ((ncor_pointer_to_check) == NULL) {              \
>>> JLI_ReportErrorMessage(JNI_ERROR);                \
>>>        return ncor_failure_return_value;                 \
>>>      }                                 \
>>>    } while(0)
>>>
>>>
>>>
>>> On Thu, May 2, 2013 at 11:16 AM, Martin Buchholz <martinrb at google.com
>>> <mailto:martinrb at google.com>> wrote:
>>>
>>>      What would Martin actually do?
>>>
>>>      #define NULL_CHECK_OR_RETURN(ncor_pointer_to_check,     \
>>>                 ncor_failure_return_value) \
>>>        do {                                        \
>>>          if ((ncor_pointer_to_check) == NULL) {              \
>>>      JLI_ReportErrorMessage(JNI_ERROR);                \
>>>            return ncor_failure_return_value;                 \
>>>          }                                       \
>>>        } while(0);
>>>
>>>
>>>
>>>
>>>      On Thu, May 2, 2013 at 10:45 AM, Kumar Srinivasan
>>>      <kumar.x.srinivasan at oracle.com
>>>      <mailto:kumar.x.srinivasan at oracle.com>> wrote:
>>>
>>>          On 5/2/2013 10:17 AM, Martin Buchholz wrote:
>>>>          This is global fix creep, but ...
>>>          :(
>>>
>>>
>>>>          these macros are also found in the hotspot sources.
>>>>          I would rewrite all the macros in the jdk to adopt the
>>>>          blessed style
>>>>          do { ... } while(0)
>>>>          and remove all comments in the jdk of the form
>>>>           /* next token must be ; */
>>>>          If you want a macro that does nothing at all, you should
>>>>          define it
>>>>          do {} while (0)
>>>>          instead of defining it to the empty string.
>>>          I am not following, could you be more explicit on how this
>>>          applies to
>>>
>>>          -#define NULL_CHECK0(e) if ((e) == 0) { \
>>>          +#define NULL_CHECK_RV(e, rv) if ((e) == 0) { \
>>>                JLI_ReportErrorMessage(JNI_ERROR); \
>>>          -    return 0; \
>>>          +    return rv; \
>>>              }
>>>
>>>          -#define NULL_CHECK(e) if ((e) == 0) { \
>>>          -    JLI_ReportErrorMessage(JNI_ERROR); \
>>>          -    return; \
>>>          -  }
>>>          +#define NULL_CHECK0(e) NULL_CHECK_RV(e, 0)
>>>
>>>          +#define NULL_CHECK(e) NULL_CHECK_RV(e, )
>>>          +
>>>
>>>
>>>>
>>>>              I would also be inclined to change
>>>>              == 0
>>>>              to
>>>>              == NULL
>>>>
>>>          Yes will take care of this, as well as Alan suggestion added a
>>>          check for malloc return.
>>>
>>>
>>>>              This seems like another occasion to use the weird
>>>>
>>>>              do { ... } while(0)
>>>>
>>>>              trick to make the macro behave more like a statement.
>>>>
>>>>              I might obfuscate the macro parameters to make collisions
>>>>              less likely, e.g. e => N_C_RV_e
>>>>
>>>          You want me to rename the macro parameter e to N_C_RV_e ? or
>>>          something else
>>>          say ncrve to avoid collision ?
>>>
>>>          Kumar
>>>
>>>
>>>>
>>>>
>>>>
>>>>              On Wed, May 1, 2013 at 12:33 PM, Kumar Srinivasan
>>>>              <kumar.x.srinivasan at oracle.com
>>>>              <mailto:kumar.x.srinivasan at oracle.com>> wrote:
>>>>
>>>>                  Hi,
>>>>
>>>>                  Please review simple fixes for code correctness in
>>>>                  the launcher.
>>>>
>>>>                  http://cr.openjdk.java.net/~ksrini/8013736/webrev.0/
>>>>                  <http://cr.openjdk.java.net/%7Eksrini/8013736/webrev.0/>
>>>>
>>>>                  Thanks
>>>>                  Kumar
>>>>
>>>>
>>>>
>>>
>>>
>>