From patchwork Sat Sep 15 09:46:50 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tobias Burnus X-Patchwork-Id: 184074 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id F2F292C0089 for ; Sat, 15 Sep 2012 19:47:16 +1000 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1348307237; h=Comment: DomainKey-Signature:Received:Received:Received:Received: Message-ID:Date:From:User-Agent:MIME-Version:To:Subject: Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:Sender:Delivered-To; bh=iFS5qit C1J/pD+BR2suDStqXVts=; b=MDgSkfx+auc6yQJbqU+doe8Trar6reoEZKUzcpS FVLRWUcCspntCAKtw7EQhIQ5qZc7fNa2gd19WjOIV2V/8RE2HOLSuVg65AwaHpWt 9qBX3o4spNA0oSEWjZTtSPepuUo5Wqg8b4qGye54lGzr2FbT0uWSD70GYLiYBHhk cFsU= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Message-ID:Date:From:User-Agent:MIME-Version:To:Subject:Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=qNgFO/2YKzEkfhlrNuZWtSvjxNVBCQqY89iodZPVwVsy6LGVGHqo4lPQbv7VtM 4cHK95a6A9eN1Os7v6AZOp+ceGu4ZyJZJBIwNQOMt8u4yS7DrIwH0LYBLSERyBfE XXtBmubHtVgNL8gM7BsGDnEPVs3C+TztbyOBGV6VmI/K4=; Received: (qmail 21923 invoked by alias); 15 Sep 2012 09:47:12 -0000 Received: (qmail 21906 invoked by uid 22791); 15 Sep 2012 09:47:10 -0000 X-SWARE-Spam-Status: No, hits=-1.8 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_NONE, TW_BG, TW_TM X-Spam-Check-By: sourceware.org Received: from mx01.qsc.de (HELO mx01.qsc.de) (213.148.129.14) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sat, 15 Sep 2012 09:46:54 +0000 Received: from [192.168.178.22] (port-92-204-67-8.dynamic.qsc.de [92.204.67.8]) by mx01.qsc.de (Postfix) with ESMTP id C00303E5F3; Sat, 15 Sep 2012 11:46:51 +0200 (CEST) Message-ID: <50544E8A.5040704@net-b.de> Date: Sat, 15 Sep 2012 11:46:50 +0200 From: Tobias Burnus User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120825 Thunderbird/15.0 MIME-Version: 1.0 To: gcc patches , gfortran Subject: [Patch, Fortran] Fix some issues found by Coverity's static-code analysis scan Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Dear all, this patch fixes some of the warning showing up for gcc/fortran at http://scan.coverity.com/. Coverity sells static C/C++/C#/Java code analyzers and offer scanning to open-source projects. The result looks like http://www.coverity.com/images/products/static-analysis/screenshot-1-large.png For GCC with a C,C++,Fortran,LTO build it shows 12,000 "defects", many are false positives, e.g. for gcc/fortran three times a warning that default: gcc_unreachable(); is unreachable as all cases have been covered. If they were covered via "case", one could remove the default and let -Wswitch -Werror do the work. But in those examples, there are also some additional "if" involved, which are before the "switch" - only with those the "default" is unreachable.Thus, the "default" has to stay. The new version of scan.coverity.com allows the project to upload the builds themselves - which is the reason that now also Fortran is supported. I done a manual build and uploaded it; it could be automatized, but that only makes sense if someone regularly looks at the results. In any case, if you have already access to scan.coverity.com, feel free to log in and look at the results. If not, you can create a new account. (I don't know whether I can use the build-upload password to create accounts, whether someone else has a project account password or whether Coverity has to handle it themselves.) I have now fixed some of the issues; please have a close look whether they make sense. With static analysis as with compiler warnings, one sometimes tends to fix them bliendly - causing wrong-code issues that way. I also tried to only fix code which is either wrong or bad style; I have ignore more bogus errors. Doing so, I have also added some gcc_asserts; one could decide that those aren't needed and if a NULL derference ever occurs, segfaulting (with the new libbacktrace stack trace) gives is sufficient feedback. Overview about the changes: * [Memory leak] arith.c's arith_power: We have "result = gfc_get_constant_expr (...);", which has to be freed in case of error - which we did only in one error case. For some reason, the scanner found only the second problem in BT_COMPLEX but not the one in BT_REAL. * [Out of bounds] array.c's gfc_match_array_ref: I am not sure whether that code is unreachable, but in any case it is out of bounds. It does not seem to trigger for our test cases; maybe removing it and adding an assert is sufficient? * [Uninitialized memory] array.c's gfc_match_array_constructor: gfc_match_decl_type_spec checks whether "ts->kind == -1" thus we to initialize it - either directly or as I did with the sledgehammer. * [dereferenced + later NULL check] array.c's gfc_resolve_character_array_constructor: The code has: if (expr->ts.u.cl == NULL) expr->ts.u.cl = gfc_new_charlen (gfc_current_ns, NULL); if (expr->ts.u.cl->length == NULL) else gcc_assert (expr->ts.u.cl->length); As "...->cl->length" is dereferenced, doesn't make sense to check again whether "...->cl" is NULL. (Besides, the code flow shows that it shouldn't.) * [dereferenced + later NULL check] array.c's gfc_array_dimen_size: Similar issue, but this time I added a gcc_assert. * [Same check twice] check.c's numeric_check: Here, one firsts returns if the type is numeric, if not, one tries implicit typing. I think the second EXPR_VARIABLE should have been EXPR_FUNCTION. * [dereferenced + later NULL check] class.c's gfc_build_class_symbol. We later (for "rank = " check whether "as" exists. I think all calls are of the form "&sym->as", but I still added an assert. * [memory leak] class.c's finalize_component: The "e" is created as copy_expr with main component ref added. It is either used in the DEALLOCATE or FINAL call, or passed in the loop for subcomponent finalization - as those use gfc_copy_expr, it has to be freed in that case. * [dereferenced + later NULL check] dump-parse-tree.c's show_namespace. I think the calls are all pointing to non-NULL namespace, but to be careful, I added an assert and removed the check. (Most changes are pure re-indenting.) * [Memory leak] trans-io.c's transfer_namelist_element: One had code like if (ts->type == BT_DERIVED) tree expr = build_fold_indirect_ref_loc (input_location, for (cmp = ts->u.derived->components; cmp; cmp = cmp->next) transfer_namelist_element (block, full_name, NULL, cmp, expr); However, when the derived type has no components, the "expr" is leaking. As Fortran 2003 allows empty components, that can actually occur in reality. * [Memory leak] trans-io.c's transfer_expr: The same issue. * [Uninitialized memory] trans-io.c's gfc_trans_transfer: Here, the warning is about accessing uninitialized loop elements in gfc_cleanup_loop. The reason is that those aren't initialized for expr->rank == 0; I thought that the code should be unreachable due to the (se.ss != 0) condition, but an assert fails for instance for graphite/pr36286.f90. On the other hand, for se.ss != NULL && expr->rank == 0, handling lower code part is wrong as it only deals with "loop". Probably, one needs to use "if (se.ss == NULL || expr->rank == 0)", which I now did. * [NULL-pointer access?] trans-io.c's gfc_trans_transfer: The issue should not occur, but to be safe and to silence the warning. The problem the scanner sees is that "ref" can be NULL if there is no ref->type == REF_ARRAY. * [Memory leakage] trans.c's gfc_trans_runtime_check: We forget to call va_end. I think that covers about 1/3 of the gcc/fortran warnings. By the way, there are no libgfortran warnings. Most of the remaining issues are about memory leakages (or bogus), but for the memory-leak ones require a closer look as the analysis jumps through the whole code. TODO: There are also real issues like the following in symbol.c's gfc_undo_symbols: gfc_free_namelist (old->namelist_tail); old->namelist_tail->next = NULL; where one first frees the memory and then sets a component to NULL. Locally, I was wondering whether the two lines should be swapped as free_namelist also frees ns->next. Alternatively, the "->next" part is wrong, which is more in line with the next line of code: p->namelist_tail = old->namelist_tail; But I think one needs to study the code more closely to know what the code is supposed to do. (Unfortunately, there does not seem to be a match for module.c. I know that there are memory leaks but the code is written such that one has no idea about the flow - especially as a lot is done via "void *" pointers.) Build and regtested on x86-64-linux. OK for the trunk? Tobias 2012-09-15 Tobias Burnus * arith.c (arith_power): Call gfc_free_expr in case of error. * array.c (gfc_match_array_ref): Fix out-of-bounds issue. (gfc_match_array_constructor): Initialize variable. (gfc_resolve_character_array_constructor): Remove superfluous check. (gfc_array_dimen_size): Add assert. * check.c (numeric_check): Fix implicit typing. * class.c (gfc_build_class_symbol): Add assert. (finalize_component): Free memory. * dump-parse-tree.c (show_namespace): Add assert. * trans-io.c (transfer_namelist_element, transfer_expr): Avoid memory leakage. (gfc_trans_transfer): Add asserts, fix condition. * trans.c (gfc_trans_runtime_check): Call va_end diff --git a/gcc/fortran/arith.c b/gcc/fortran/arith.c index 6fa7c70..e94566a 100644 --- a/gcc/fortran/arith.c +++ b/gcc/fortran/arith.c @@ -906,7 +906,10 @@ arith_power (gfc_expr *op1, gfc_expr *op2, gfc_expr **resultp) if (gfc_notify_std (GFC_STD_F2003, "Noninteger " "exponent in an initialization " "expression at %L", &op2->where) == FAILURE) - return ARITH_PROHIBIT; + { + gfc_free_expr (result); + return ARITH_PROHIBIT; + } } if (mpfr_cmp_si (op1->value.real, 0) < 0) @@ -928,7 +931,10 @@ arith_power (gfc_expr *op1, gfc_expr *op2, gfc_expr **resultp) if (gfc_notify_std (GFC_STD_F2003, "Noninteger " "exponent in an initialization " "expression at %L", &op2->where) == FAILURE) - return ARITH_PROHIBIT; + { + gfc_free_expr (result); + return ARITH_PROHIBIT; + } } mpc_pow (result->value.complex, op1->value.complex, diff --git a/gcc/fortran/array.c b/gcc/fortran/array.c index 44ec72e..1611c3b 100644 --- a/gcc/fortran/array.c +++ b/gcc/fortran/array.c @@ -253,7 +253,7 @@ coarray: gfc_error ("Invalid form of coarray reference at %C"); return MATCH_ERROR; } - else if (ar->dimen_type[ar->codimen + ar->dimen] == DIMEN_STAR) + else if (ar->dimen_type[ar->codimen + ar->dimen - 1] == DIMEN_STAR) { gfc_error ("Unexpected '*' for codimension %d of %d at %C", ar->codimen + 1, corank); @@ -1074,6 +1074,7 @@ gfc_match_array_constructor (gfc_expr **result) seen_ts = false; /* Try to match an optional "type-spec ::" */ + gfc_clear_ts (&ts); if (gfc_match_decl_type_spec (&ts, 0) == MATCH_YES) { seen_ts = (gfc_match (" ::") == MATCH_YES); @@ -1973,7 +1974,7 @@ got_charlen: /* If gfc_extract_int above set current_length, we implicitly know the type is BT_INTEGER and it's EXPR_CONSTANT. */ - has_ts = (expr->ts.u.cl && expr->ts.u.cl->length_from_typespec); + has_ts = expr->ts.u.cl->length_from_typespec; if (! cl || (current_length != -1 && current_length != found_length)) @@ -2225,13 +2226,15 @@ gfc_array_dimen_size (gfc_expr *array, int dimen, mpz_t *result) gfc_ref *ref; int i; + gcc_assert (array != NULL); + if (array->ts.type == BT_CLASS) return FAILURE; if (array->rank == -1) return FAILURE; - if (dimen < 0 || array == NULL || dimen > array->rank - 1) + if (dimen < 0 || dimen > array->rank - 1) gfc_internal_error ("gfc_array_dimen_size(): Bad dimension"); switch (array->expr_type) diff --git a/gcc/fortran/check.c b/gcc/fortran/check.c index 2235b52..58c5856 100644 --- a/gcc/fortran/check.c +++ b/gcc/fortran/check.c @@ -79,7 +79,7 @@ numeric_check (gfc_expr *e, int n) /* If the expression has not got a type, check if its namespace can offer a default type. */ - if ((e->expr_type == EXPR_VARIABLE || e->expr_type == EXPR_VARIABLE) + if ((e->expr_type == EXPR_VARIABLE || e->expr_type == EXPR_FUNCTION) && e->symtree->n.sym->ts.type == BT_UNKNOWN && gfc_set_default_type (e->symtree->n.sym, 0, e->symtree->n.sym->ns) == SUCCESS diff --git a/gcc/fortran/class.c b/gcc/fortran/class.c index dca2cfc..2e347cb 100644 --- a/gcc/fortran/class.c +++ b/gcc/fortran/class.c @@ -503,7 +503,9 @@ gfc_build_class_symbol (gfc_typespec *ts, symbol_attribute *attr, gfc_component *c; int rank; - if (as && *as && (*as)->type == AS_ASSUMED_SIZE) + gcc_assert (as); + + if (*as && (*as)->type == AS_ASSUMED_SIZE) { gfc_error ("Assumed size polymorphic objects or components, such " "as that at %C, have not yet been implemented"); @@ -838,6 +840,7 @@ finalize_component (gfc_expr *expr, gfc_symbol *derived, gfc_component *comp, for (c = comp->ts.u.derived->components; c; c = c->next) finalize_component (e, c->ts.u.derived, c, stat, code); + gfc_free_expr (e); } } diff --git a/gcc/fortran/dump-parse-tree.c b/gcc/fortran/dump-parse-tree.c index 9d6f93c..a442625 100644 --- a/gcc/fortran/dump-parse-tree.c +++ b/gcc/fortran/dump-parse-tree.c @@ -2248,67 +2248,63 @@ show_namespace (gfc_namespace *ns) gfc_equiv *eq; int i; + gcc_assert (ns); save = gfc_current_ns; show_indent (); fputs ("Namespace:", dumpfile); - if (ns != NULL) + i = 0; + do { - i = 0; - do - { - int l = i; - while (i < GFC_LETTERS - 1 - && gfc_compare_types(&ns->default_type[i+1], - &ns->default_type[l])) - i++; - - if (i > l) - fprintf (dumpfile, " %c-%c: ", l+'A', i+'A'); - else - fprintf (dumpfile, " %c: ", l+'A'); + int l = i; + while (i < GFC_LETTERS - 1 + && gfc_compare_types (&ns->default_type[i+1], + &ns->default_type[l])) + i++; + + if (i > l) + fprintf (dumpfile, " %c-%c: ", l+'A', i+'A'); + else + fprintf (dumpfile, " %c: ", l+'A'); - show_typespec(&ns->default_type[l]); - i++; - } while (i < GFC_LETTERS); + show_typespec(&ns->default_type[l]); + i++; + } while (i < GFC_LETTERS); - if (ns->proc_name != NULL) - { - show_indent (); - fprintf (dumpfile, "procedure name = %s", ns->proc_name->name); - } + if (ns->proc_name != NULL) + { + show_indent (); + fprintf (dumpfile, "procedure name = %s", ns->proc_name->name); + } - ++show_level; - gfc_current_ns = ns; - gfc_traverse_symtree (ns->common_root, show_common); + ++show_level; + gfc_current_ns = ns; + gfc_traverse_symtree (ns->common_root, show_common); - gfc_traverse_symtree (ns->sym_root, show_symtree); + gfc_traverse_symtree (ns->sym_root, show_symtree); - for (op = GFC_INTRINSIC_BEGIN; op != GFC_INTRINSIC_END; op++) - { - /* User operator interfaces */ - intr = ns->op[op]; - if (intr == NULL) - continue; + for (op = GFC_INTRINSIC_BEGIN; op != GFC_INTRINSIC_END; op++) + { + /* User operator interfaces */ + intr = ns->op[op]; + if (intr == NULL) + continue; - show_indent (); - fprintf (dumpfile, "Operator interfaces for %s:", - gfc_op2string ((gfc_intrinsic_op) op)); + show_indent (); + fprintf (dumpfile, "Operator interfaces for %s:", + gfc_op2string ((gfc_intrinsic_op) op)); - for (; intr; intr = intr->next) - fprintf (dumpfile, " %s", intr->sym->name); - } + for (; intr; intr = intr->next) + fprintf (dumpfile, " %s", intr->sym->name); + } - if (ns->uop_root != NULL) - { - show_indent (); - fputs ("User operators:\n", dumpfile); - gfc_traverse_user_op (ns, show_uop); - } + if (ns->uop_root != NULL) + { + show_indent (); + fputs ("User operators:\n", dumpfile); + gfc_traverse_user_op (ns, show_uop); } - else - ++show_level; for (eq = ns->equiv; eq; eq = eq->next) show_equiv (eq); diff --git a/gcc/fortran/trans-io.c b/gcc/fortran/trans-io.c index 34db6fd..4ad961f 100644 --- a/gcc/fortran/trans-io.c +++ b/gcc/fortran/trans-io.c @@ -1611,7 +1611,7 @@ transfer_namelist_element (stmtblock_t * block, const char * var_name, gfc_add_expr_to_block (block, tmp); } - if (ts->type == BT_DERIVED) + if (ts->type == BT_DERIVED && ts->u.derived->components) { gfc_component *cmp; @@ -2146,6 +2146,9 @@ transfer_expr (gfc_se * se, gfc_typespec * ts, tree addr_expr, gfc_code * code) break; case BT_DERIVED: + if (ts->u.derived->components == NULL) + return; + /* Recurse into the elements of the derived type. */ expr = gfc_evaluate_now (addr_expr, &se->pre); expr = build_fold_indirect_ref_loc (input_location, @@ -2251,8 +2254,8 @@ gfc_trans_transfer (gfc_code * code) if (expr->ref && !gfc_is_proc_ptr_comp (expr)) { for (ref = expr->ref; ref && ref->type != REF_ARRAY; - ref = ref->next); - gcc_assert (ref->type == REF_ARRAY); + ref = ref->next); + gcc_assert (ref && ref->type == REF_ARRAY); } if (expr->ts.type != BT_DERIVED @@ -2310,7 +2313,7 @@ gfc_trans_transfer (gfc_code * code) gfc_add_block_to_block (&body, &se.pre); gfc_add_block_to_block (&body, &se.post); - if (se.ss == NULL) + if (se.ss == NULL || expr->rank == 0) tmp = gfc_finish_block (&body); else { diff --git a/gcc/fortran/trans.c b/gcc/fortran/trans.c index ff0b243..6365213 100644 --- a/gcc/fortran/trans.c +++ b/gcc/fortran/trans.c @@ -506,6 +506,7 @@ gfc_trans_runtime_check (bool error, bool once, tree cond, stmtblock_t * pblock, gfc_add_expr_to_block (&block, trans_runtime_error_vararg (error, where, msgid, ap)); + va_end (ap); if (once) gfc_add_modify (&block, tmpvar, boolean_false_node);