RFR: JDK-8013736: [launcher] cleanup code for correctness (original) (raw)

Martin Buchholz martinrb at google.com
Tue May 7 02:38:33 UTC 2013

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. 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 NULLCHECKORRETURN(ncorpointertocheck, _ _ncorfailurereturnvalue) _ _do { _ _if ((ncorpointertocheck) == NULL) { _ _JLIReportErrorMessage(JNIERROR); _ _return ncorfailurereturnvalue; _ _} _ } 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 NULLCHECKORRETURN(ncorpointertocheck, _ _ncorfailurereturnvalue) _ _do { _ _if ((ncorpointertocheck) == NULL) { _ _JLIReportErrorMessage(JNIERROR); _ _return ncorfailurereturnvalue; _ _} _ } 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 NULLCHECK0(e) if ((e) == 0) { _ _+#define NULLCHECKRV(e, rv) if ((e) == 0) { _ _JLIReportErrorMessage(JNIERROR); _ _- return 0; _ _+ return rv; _ } _-#define NULLCHECK(e) if ((e) == 0) { _ _- JLIReportErrorMessage(JNIERROR); _ _- return; _ - } +#define NULLCHECK0(e) NULLCHECKRV(e, 0) +#define NULLCHECK(e) NULLCHECKRV(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 => NCRVe You want me to rename the macro parameter e to NCRVe ? 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

More information about the core-libs-dev mailing list