From patchwork Tue Dec 6 18:38:00 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 61604 Return-Path: X-Original-To: patchwork@sourceware.org Delivered-To: patchwork@sourceware.org Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id B7214383B6DD for ; Tue, 6 Dec 2022 18:39:14 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B7214383B6DD DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1670351954; bh=gNBYAWko+EOLlosry1iXup8stfR7Aohs0C19ctqfYr0=; h=To:Cc:Subject:Date:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=ek1zKawXyLDUFKAEeMxrRUB0lvlU7rNEWg7jfb+KWvGPysSil9W7lKUV4lAQhy9BJ 0SOT/5jX9sdi246UZnv0Lo4Qnqqb7Sf4PW0EE7xXLD2HJzKTUq1GTpuIDIh0222S3Z fh2Uqa4TenAUyuBUQ3rhstx+rgljHSiTyjHucycQ= X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTPS id 49BDC3839D1A for ; Tue, 6 Dec 2022 18:38:06 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 49BDC3839D1A Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-621-sxEwgRS2N5eYvsMb7VnUOw-1; Tue, 06 Dec 2022 13:38:04 -0500 X-MC-Unique: sxEwgRS2N5eYvsMb7VnUOw-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 05EF7185A78B for ; Tue, 6 Dec 2022 18:38:04 +0000 (UTC) Received: from t14s.localdomain.com (unknown [10.2.17.99]) by smtp.corp.redhat.com (Postfix) with ESMTP id CB70D40C6EC2; Tue, 6 Dec 2022 18:38:03 +0000 (UTC) To: gcc-patches@gcc.gnu.org Cc: David Malcolm Subject: [committed] analyzer: use __attribute__((nonnull)) at top level of analysis [PR106325] Date: Tue, 6 Dec 2022 13:38:00 -0500 Message-Id: <20221206183800.4095931-1-dmalcolm@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.2 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.4 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2, SPF_HELO_NONE, SPF_NONE, TXREP autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-Patchwork-Original-From: David Malcolm via Gcc-patches From: David Malcolm Reply-To: David Malcolm Errors-To: gcc-patches-bounces+patchwork=sourceware.org@gcc.gnu.org Sender: "Gcc-patches" PR analyzer/106325 reports false postives from -Wanalyzer-null-dereference on code like this: __attribute__((nonnull)) void foo_a (Foo *p) { foo_b (p); switch (p->type) { /* ... */ } } where foo_b (p) has a: g_return_if_fail (p); that expands to: if (!p) { return; } The analyzer "sees" the comparison against NULL in foo_b, and splits the analysis into the NULL and not-NULL cases; later, back in foo_a, at switch (p->type) it complains that p is NULL. Previously we were only using __attribute__((nonnull)) as something to complain about when it was violated; we weren't using it as a source of knowledge. This patch fixes things by making the analyzer respect __attribute__((nonnull)) at the top-level of the analysis: any such params are now assumed to be non-NULL, so that the analyzer assumes the g_return_if_fail inside foo_b doesn't fail when called from foo_a Doing so fixes the false positives. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. Pushed to trunk as r13-4520-gdcfc7ac94dbcf6. gcc/analyzer/ChangeLog: PR analyzer/106325 * region-model-manager.cc (region_model_manager::get_or_create_null_ptr): New. * region-model-manager.h (region_model_manager::get_or_create_null_ptr): New decl. * region-model.cc (region_model::on_top_level_param): Add "nonnull" param and make use of it. (region_model::push_frame): When handling a top-level entrypoint to the analysis, determine which params __attribute__((nonnull)) applies to, and pass to on_top_level_param. * region-model.h (region_model::on_top_level_param): Add "nonnull" param. gcc/testsuite/ChangeLog: PR analyzer/106325 * gcc.dg/analyzer/attr-nonnull-pr106325.c: New test. * gcc.dg/analyzer/attribute-nonnull.c (test_6): New. (test_7): New. Signed-off-by: David Malcolm --- gcc/analyzer/region-model-manager.cc | 11 + gcc/analyzer/region-model-manager.h | 1 + gcc/analyzer/region-model.cc | 29 +- gcc/analyzer/region-model.h | 4 +- .../gcc.dg/analyzer/attr-nonnull-pr106325.c | 250 ++++++++++++++++++ .../gcc.dg/analyzer/attribute-nonnull.c | 18 ++ 6 files changed, 308 insertions(+), 5 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/attr-nonnull-pr106325.c diff --git a/gcc/analyzer/region-model-manager.cc b/gcc/analyzer/region-model-manager.cc index 471a9272e41..0fb96386f28 100644 --- a/gcc/analyzer/region-model-manager.cc +++ b/gcc/analyzer/region-model-manager.cc @@ -237,6 +237,17 @@ region_model_manager::get_or_create_int_cst (tree type, poly_int64 val) return get_or_create_constant_svalue (tree_cst); } +/* Return the svalue * for the constant_svalue for the NULL pointer + of POINTER_TYPE, creating it if necessary. */ + +const svalue * +region_model_manager::get_or_create_null_ptr (tree pointer_type) +{ + gcc_assert (pointer_type); + gcc_assert (POINTER_TYPE_P (pointer_type)); + return get_or_create_int_cst (pointer_type, 0); +} + /* Return the svalue * for a unknown_svalue for TYPE (which can be NULL), creating it if necessary. The unknown_svalue instances are reused, based on pointer equality diff --git a/gcc/analyzer/region-model-manager.h b/gcc/analyzer/region-model-manager.h index 22f980056fa..13fbe483f6d 100644 --- a/gcc/analyzer/region-model-manager.h +++ b/gcc/analyzer/region-model-manager.h @@ -43,6 +43,7 @@ public: /* svalue consolidation. */ const svalue *get_or_create_constant_svalue (tree cst_expr); const svalue *get_or_create_int_cst (tree type, poly_int64); + const svalue *get_or_create_null_ptr (tree pointer_type); const svalue *get_or_create_unknown_svalue (tree type); const svalue *get_or_create_setjmp_svalue (const setjmp_record &r, tree type); diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index d00f15f468f..430c0e91175 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -4385,11 +4385,13 @@ region_model::apply_constraints_for_exception (const gimple *last_stmt, PARAM has a defined but unknown initial value. Anything it points to has escaped, since the calling context "knows" the pointer, and thus calls to unknown functions could read/write into - the region. */ + the region. + If NONNULL is true, then assume that PARAM must be non-NULL. */ void region_model::on_top_level_param (tree param, - region_model_context *ctxt) + bool nonnull, + region_model_context *ctxt) { if (POINTER_TYPE_P (TREE_TYPE (param))) { @@ -4398,6 +4400,12 @@ region_model::on_top_level_param (tree param, = m_mgr->get_or_create_initial_value (param_reg); const region *pointee_reg = m_mgr->get_symbolic_region (init_ptr_sval); m_store.mark_as_escaped (pointee_reg); + if (nonnull) + { + const svalue *null_ptr_sval + = m_mgr->get_or_create_null_ptr (TREE_TYPE (param)); + add_constraint (init_ptr_sval, NE_EXPR, null_ptr_sval, ctxt); + } } } @@ -4453,14 +4461,27 @@ region_model::push_frame (function *fun, const vec *arg_svals, have defined but unknown initial values. Anything they point to has escaped. */ tree fndecl = fun->decl; + + /* Handle "__attribute__((nonnull))". */ + tree fntype = TREE_TYPE (fndecl); + bitmap nonnull_args = get_nonnull_args (fntype); + + unsigned parm_idx = 0; for (tree iter_parm = DECL_ARGUMENTS (fndecl); iter_parm; iter_parm = DECL_CHAIN (iter_parm)) { + bool non_null = (nonnull_args + ? (bitmap_empty_p (nonnull_args) + || bitmap_bit_p (nonnull_args, parm_idx)) + : false); if (tree parm_default_ssa = ssa_default_def (fun, iter_parm)) - on_top_level_param (parm_default_ssa, ctxt); + on_top_level_param (parm_default_ssa, non_null, ctxt); else - on_top_level_param (iter_parm, ctxt); + on_top_level_param (iter_parm, non_null, ctxt); + parm_idx++; } + + BITMAP_FREE (nonnull_args); } return m_current_frame; diff --git a/gcc/analyzer/region-model.h b/gcc/analyzer/region-model.h index 3b93d3e16b8..291bb2ff45a 100644 --- a/gcc/analyzer/region-model.h +++ b/gcc/analyzer/region-model.h @@ -530,7 +530,9 @@ private: int poison_any_pointers_to_descendents (const region *reg, enum poison_kind pkind); - void on_top_level_param (tree param, region_model_context *ctxt); + void on_top_level_param (tree param, + bool nonnull, + region_model_context *ctxt); bool called_from_main_p () const; const svalue *get_initial_value_for_global (const region *reg) const; diff --git a/gcc/testsuite/gcc.dg/analyzer/attr-nonnull-pr106325.c b/gcc/testsuite/gcc.dg/analyzer/attr-nonnull-pr106325.c new file mode 100644 index 00000000000..3b264719f6d --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/attr-nonnull-pr106325.c @@ -0,0 +1,250 @@ +typedef long int signed_frame_t; + +typedef struct Track Track; +typedef struct ZRegion ZRegion; +typedef struct AutomationTrack AutomationTrack; +typedef struct MidiNote MidiNote; +typedef struct ArrangerObject ArrangerObject; +typedef struct Project Project; +typedef struct ZRegion ZRegion; +typedef struct Position Position; +typedef struct Track Track; +typedef struct ClipEditor ClipEditor; + +typedef enum ArrangerObjectType +{ + /* .... */ + ARRANGER_OBJECT_TYPE_REGION, + ARRANGER_OBJECT_TYPE_MIDI_NOTE, + /* .... */ +} ArrangerObjectType; + +typedef enum ArrangerObjectPositionType +{ + ARRANGER_OBJECT_POSITION_TYPE_START, + ARRANGER_OBJECT_POSITION_TYPE_END, + ARRANGER_OBJECT_POSITION_TYPE_CLIP_START, + ARRANGER_OBJECT_POSITION_TYPE_LOOP_START, + ARRANGER_OBJECT_POSITION_TYPE_LOOP_END, + ARRANGER_OBJECT_POSITION_TYPE_FADE_IN, + ARRANGER_OBJECT_POSITION_TYPE_FADE_OUT, +} ArrangerObjectPositionType; + +typedef struct Position +{ + /* .... */ + double ticks; + /* .... */ +} Position; + +typedef enum RegionType +{ + /* .... */ + REGION_TYPE_AUTOMATION = 1 << 2, + /* .... */ +} RegionType; + +typedef struct RegionIdentifier +{ + /* .... */ + RegionType type; + /* .... */ + int lane_pos; + /* .... */ +} RegionIdentifier; + +typedef struct ArrangerObject +{ + /* .... */ + + ArrangerObjectType type; + /* .... */ + Position pos; + Position end_pos; + Position clip_start_pos; + + Position loop_start_pos; + Position loop_end_pos; + + Position fade_in_pos; + Position fade_out_pos; + + /* .... */ +} ArrangerObject; + +typedef struct ZRegion +{ + /* .... */ + RegionIdentifier id; + /* .... */ + int num_midi_notes; + /* .... */ +} ZRegion; + +typedef struct Zrythm +{ + /* ... */ + Project *project; + /* ... */ +} Zrythm; + +typedef struct Project +{ + /* ... */ + + ClipEditor *clip_editor; + + /* ... */ +} Project; + +extern Zrythm *zrythm; + +extern void g_return_if_fail_warning (const char *log_domain, + const char *pretty_function, + const char *expression); +extern void position_add_ticks (Position *self, double ticks); +extern _Bool +arranger_object_is_position_valid (const ArrangerObject *const self, + const Position *pos, + ArrangerObjectPositionType pos_type); +extern Track *arranger_object_get_track (const ArrangerObject *const self); +extern void midi_region_insert_midi_note (ZRegion *region, MidiNote *midi_note, + int idx, int pub_events); +extern ZRegion *midi_note_get_region (MidiNote *self); +extern AutomationTrack * +region_get_automation_track (const ZRegion *const region); +extern void track_add_region (Track *track, ZRegion *region, + AutomationTrack *at, int lane_pos, int gen_name, + int fire_events); +extern void clip_editor_set_region (ClipEditor *self, ZRegion *region, + _Bool fire_events); +extern ZRegion *clip_editor_get_region (ClipEditor *self); + +static Position * +get_position_ptr (ArrangerObject *self, ArrangerObjectPositionType pos_type) +{ + switch (pos_type) + { + case ARRANGER_OBJECT_POSITION_TYPE_START: + return &self->pos; + case ARRANGER_OBJECT_POSITION_TYPE_END: + return &self->end_pos; + case ARRANGER_OBJECT_POSITION_TYPE_CLIP_START: + return &self->clip_start_pos; + case ARRANGER_OBJECT_POSITION_TYPE_LOOP_START: + return &self->loop_start_pos; + case ARRANGER_OBJECT_POSITION_TYPE_LOOP_END: + return &self->loop_end_pos; + case ARRANGER_OBJECT_POSITION_TYPE_FADE_IN: + return &self->fade_in_pos; + case ARRANGER_OBJECT_POSITION_TYPE_FADE_OUT: + return &self->fade_out_pos; + } + return (((void *)0)); +} + +void +arranger_object_set_position (ArrangerObject *self, const Position *pos, + ArrangerObjectPositionType pos_type, + const int validate) +{ + if (!(self && pos)) + { + g_return_if_fail_warning ("zrythm", ((const char *)(__func__)), + "self && pos"); + return; + } + + if (validate && !arranger_object_is_position_valid (self, pos, pos_type)) + return; + + Position *pos_ptr; + pos_ptr = get_position_ptr (self, pos_type); + if (!pos_ptr) + { + g_return_if_fail_warning ("zrythm", ((const char *)(__func__)), + "pos_ptr"); + return; + } + *(pos_ptr) = *(pos); +} + +void +arranger_object_end_pos_setter (ArrangerObject *self, const Position *pos) +{ + arranger_object_set_position (self, pos, ARRANGER_OBJECT_POSITION_TYPE_END, + 1); +} + +ArrangerObject * +arranger_object_clone (const ArrangerObject *self) +{ + if (!self) + { + g_return_if_fail_warning ("zrythm", ((const char *)(__func__)), "self"); + return (((void *)0)); + } + /* .... */ + return (((void *)0)); +} + +__attribute__((nonnull(1, 2))) +void +arranger_object_unsplit (ArrangerObject *r1, ArrangerObject *r2, + ArrangerObject **obj, _Bool fire_events) +{ + ZRegion *clip_editor_region + = clip_editor_get_region (((zrythm)->project->clip_editor)); + + _Bool set_clip_editor_region = 0; + if (clip_editor_region == (ZRegion *)r1 + || clip_editor_region == (ZRegion *)r2) + { + set_clip_editor_region = 1; + clip_editor_set_region (((zrythm)->project->clip_editor), ((void *)0), + 1); + } + + *obj = arranger_object_clone (r1); + + arranger_object_end_pos_setter (*obj, &r2->end_pos); + Position fade_out_pos; + *(&fade_out_pos) = *(&r2->end_pos); + position_add_ticks (&fade_out_pos, -r2->pos.ticks); + arranger_object_set_position (*obj, &fade_out_pos, + ARRANGER_OBJECT_POSITION_TYPE_FADE_OUT, 0); + + switch (r1->type) /* { dg-bogus "dereference of NULL 'r1'" } */ + { + case ARRANGER_OBJECT_TYPE_REGION: + { + ZRegion *r1_region = (ZRegion *)r1; + AutomationTrack *at = ((void *)0); + if (r1_region->id.type == REGION_TYPE_AUTOMATION) + { + at = region_get_automation_track (r1_region); + } + track_add_region (arranger_object_get_track (r1), (ZRegion *)*obj, at, + ((ZRegion *)r1)->id.lane_pos, 1, fire_events); + } + break; + case ARRANGER_OBJECT_TYPE_MIDI_NOTE: + { + ZRegion *parent_region = midi_note_get_region (((MidiNote *)r1)); + midi_region_insert_midi_note ( + parent_region, (MidiNote *)*obj, + ((ZRegion *)(parent_region))->num_midi_notes, 1); + } + break; + default: + break; + } + + switch (r1->type) /* { dg-bogus "dereference of NULL 'r1'" } */ + { + /* .... */ + default: + break; + } + /* .... */ +} diff --git a/gcc/testsuite/gcc.dg/analyzer/attribute-nonnull.c b/gcc/testsuite/gcc.dg/analyzer/attribute-nonnull.c index 70bb921e742..7c71a71c930 100644 --- a/gcc/testsuite/gcc.dg/analyzer/attribute-nonnull.c +++ b/gcc/testsuite/gcc.dg/analyzer/attribute-nonnull.c @@ -1,3 +1,5 @@ +#include "analyzer-decls.h" + #include extern void foo(void *ptrA, void *ptrB, void *ptrC) /* { dg-message "argument 1 of 'foo' must be non-null" } */ @@ -81,3 +83,19 @@ void test_5 (void *q, void *r) free(p); } + +__attribute__((nonnull(1, 3))) +void test_6 (void *p, void *q, void *r) +{ + __analyzer_eval (p != NULL); /* { dg-warning "TRUE" } */ + __analyzer_eval (q != NULL); /* { dg-warning "UNKNOWN" } */ + __analyzer_eval (r != NULL); /* { dg-warning "TRUE" } */ +} + +__attribute__((nonnull)) +void test_7 (void *p, void *q, void *r) +{ + __analyzer_eval (p != NULL); /* { dg-warning "TRUE" } */ + __analyzer_eval (q != NULL); /* { dg-warning "TRUE" } */ + __analyzer_eval (r != NULL); /* { dg-warning "TRUE" } */ +}