From patchwork Fri Dec 2 21:35:52 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 61402 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 786183858005 for ; Fri, 2 Dec 2022 21:36:33 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 786183858005 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gcc.gnu.org; s=default; t=1670016993; bh=KMVkiY1Mg08p5NqOtKYAgmvo+qDwwLEGGcThw+Ze/yo=; h=To:Cc:Subject:Date:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:List-Subscribe:From:Reply-To:From; b=fMCx9E1bwlmjLMVr16igu4sQ+RLFt5NoxJMS99yRMDXmcgyUXeCE/T/jJcLT9mi8r gE0lASHbMR23Xa+GpRJX9xP/cvVonlj2VdumNwl1jDQiratoltnwyEqpN2t5nuNh7/ GIuFcXg5b6/jYsqIbR87IU5LvQVHl+o59/BaOo28= 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 9E2993858C52 for ; Fri, 2 Dec 2022 21:35:58 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 9E2993858C52 Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-595-AFrx_4b3MjCIhnySC4P4vg-1; Fri, 02 Dec 2022 16:35:57 -0500 X-MC-Unique: AFrx_4b3MjCIhnySC4P4vg-1 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id B9ED43C0F424 for ; Fri, 2 Dec 2022 21:35:55 +0000 (UTC) Received: from t14s.localdomain.com (unknown [10.2.16.65]) by smtp.corp.redhat.com (Postfix) with ESMTP id 72BABC15BB4; Fri, 2 Dec 2022 21:35:55 +0000 (UTC) To: gcc-patches@gcc.gnu.org Cc: David Malcolm Subject: [committed] analyzer: fixes to region creation messages [PR107851] Date: Fri, 2 Dec 2022 16:35:52 -0500 Message-Id: <20221202213552.3820428-1-dmalcolm@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.5 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_ASCII_DIVIDERS, 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" In r13-2573-gc81b60b8c6ff3d I split up the analyzer's region-creation events to describe the memory space and capacity of the region as two separate events to avoid combinatorial explosion of message wordings. However I didn't take into account r13-1405-ge6c3bb379f515b which added a pending_diagnostic::describe_region_creation_event vfunc which could change the wording of region creation events. Hence for: #include #include void test () { int32_t *ptr = malloc (1); free (ptr); } trunk currently emits: Compiler Explorer (x86_64 trunk): https://godbolt.org/z/e3Td7c9s5: : In function 'test': :6:18: warning: allocated buffer size is not a multiple of the pointee's size [CWE-131] [-Wanalyzer-allocation-size] 6 | int32_t *ptr = malloc (1); | ^~~~~~~~~~ 'test': events 1-3 | | 6 | int32_t *ptr = malloc (1); | | ^~~~~~~~~~ | | | | | (1) allocated 1 bytes here | | (2) allocated 1 bytes here | | (3) assigned to 'int32_t *' {aka 'int *'} here; 'sizeof (int32_t {aka int})' is '4' | where events (1) and (2) are different region_creation_events that have had their wording overridden (also, with a "1 bytes" issue). This patch reorganizes region creation events so that each pending_diagnostic instead creates the events that is appropriate for it, and the events have responsibility for their own wording. With this patch, the above emits: : In function 'test': :6:18: warning: allocated buffer size is not a multiple of the pointee's size [CWE-131] [-Wanalyzer-allocation-size] 6 | int32_t *ptr = malloc (1); | ^~~~~~~~~~ 'test': events 1-2 | | 6 | int32_t *ptr = malloc (1); | | ^~~~~~~~~~ | | | | | (1) allocated 1 byte here | | (2) assigned to 'int32_t *' {aka 'int *'} here; 'sizeof (int32_t {aka int})' is '4' | fixing the duplicate event, and fixing the singular/plural issue. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. Pushed to trunk as r13-4471-gf5758fe5b430ef. gcc/analyzer/ChangeLog: PR analyzer/107851 * analyzer.cc (make_label_text_n): Convert param "n" from int to unsigned HOST_WIDE_INT. * analyzer.h (make_label_text_n): Likewise for decl. * bounds-checking.cc: Include "analyzer/checker-event.h" and "analyzer/checker-path.h". (out_of_bounds::add_region_creation_events): New. (concrete_past_the_end::describe_region_creation_event): Replace with... (concrete_past_the_end::add_region_creation_events): ...this. (symbolic_past_the_end::describe_region_creation_event): Delete. * checker-event.cc (region_creation_event::region_creation_event): Update for dropping all member data. (region_creation_event::get_desc): Delete, splitting out into region_creation_event_memory_space::get_desc, region_creation_event_capacity::get_desc, and region_creation_event_debug::get_desc. (region_creation_event_memory_space::get_desc): New. (region_creation_event_capacity::get_desc): New. (region_creation_event_allocation_size::get_desc): New. (region_creation_event_debug::get_desc): New. * checker-event.h: Include "analyzer/program-state.h". (enum rce_kind): Delete. (class region_creation_event): Drop all member data. (region_creation_event::region_creation_event): Make protected. (region_creation_event::get_desc): Delete. (class region_creation_event_memory_space): New. (class region_creation_event_capacity): New. (class region_creation_event_allocation_size): New. (class region_creation_event_debug): New. * checker-path.cc (checker_path::add_region_creation_events): Add "pd" param. Call pending_diangnostic::add_region_creation_events. Update for conversion of RCE_DEBUG to region_creation_event_debug. * checker-path.h (checker_path::add_region_creation_events): Add "pd" param. * diagnostic-manager.cc (diagnostic_manager::build_emission_path): Pass pending_diagnostic to emission_path::add_region_creation_events. (diagnostic_manager::build_emission_path): Pass path_builder to add_event_on_final_node. (diagnostic_manager::add_event_on_final_node): Add "pb" param. Pass pending_diagnostic to emission_path::add_region_creation_events. (diagnostic_manager::add_events_for_eedge): Pass pending_diagnostic to emission_path::add_region_creation_events. * diagnostic-manager.h (diagnostic_manager::add_event_on_final_node): Add "pb" param. * pending-diagnostic.cc (pending_diagnostic::add_region_creation_events): New. * pending-diagnostic.h (struct region_creation): Delete. (pending_diagnostic::describe_region_creation_event): Delete. (pending_diagnostic::add_region_creation_events): New vfunc. * region-model.cc: Include "analyzer/checker-event.h" and "analyzer/checker-path.h". (dubious_allocation_size::dubious_allocation_size): Initialize m_has_allocation_event. (dubious_allocation_size::describe_region_creation_event): Delete. (dubious_allocation_size::describe_final_event): Update for replacement of m_allocation_event with m_has_allocation_event. (dubious_allocation_size::add_region_creation_events): New. (dubious_allocation_size::m_allocation_event): Replace with... (dubious_allocation_size::m_has_allocation_event): ...this. gcc/testsuite/ChangeLog: PR analyzer/107851 * gcc.dg/analyzer/allocation-size-4.c: Update expected wording. * gcc.dg/analyzer/allocation-size-multiline-1.c: New test. * gcc.dg/analyzer/allocation-size-multiline-2.c: New test. * gcc.dg/analyzer/out-of-bounds-multiline-1.c: Update expected wording. * gcc.dg/analyzer/out-of-bounds-multiline-2.c: New test. * gcc.dg/analyzer/out-of-bounds-read-char-arr.c: Update expected wording. * gcc.dg/analyzer/out-of-bounds-read-int-arr.c: Likewise. * gcc.dg/analyzer/out-of-bounds-write-char-arr.c: Likewise. * gcc.dg/analyzer/out-of-bounds-write-int-arr.c: Likewise. Signed-off-by: David Malcolm --- gcc/analyzer/analyzer.cc | 2 +- gcc/analyzer/analyzer.h | 3 +- gcc/analyzer/bounds-checking.cc | 40 +++--- gcc/analyzer/checker-event.cc | 115 ++++++++---------- gcc/analyzer/checker-event.h | 101 ++++++++++++--- gcc/analyzer/checker-path.cc | 14 +-- gcc/analyzer/checker-path.h | 3 +- gcc/analyzer/diagnostic-manager.cc | 18 ++- gcc/analyzer/diagnostic-manager.h | 3 +- gcc/analyzer/pending-diagnostic.cc | 20 +++ gcc/analyzer/pending-diagnostic.h | 37 ++---- gcc/analyzer/region-model.cc | 47 +++---- .../gcc.dg/analyzer/allocation-size-4.c | 2 +- .../analyzer/allocation-size-multiline-1.c | 59 +++++++++ .../analyzer/allocation-size-multiline-2.c | 62 ++++++++++ .../analyzer/out-of-bounds-multiline-1.c | 2 +- .../analyzer/out-of-bounds-multiline-2.c | 32 +++++ .../analyzer/out-of-bounds-read-char-arr.c | 2 +- .../analyzer/out-of-bounds-read-int-arr.c | 2 +- .../analyzer/out-of-bounds-write-char-arr.c | 2 +- .../analyzer/out-of-bounds-write-int-arr.c | 2 +- 21 files changed, 396 insertions(+), 172 deletions(-) create mode 100644 gcc/testsuite/gcc.dg/analyzer/allocation-size-multiline-1.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/allocation-size-multiline-2.c create mode 100644 gcc/testsuite/gcc.dg/analyzer/out-of-bounds-multiline-2.c diff --git a/gcc/analyzer/analyzer.cc b/gcc/analyzer/analyzer.cc index 96497dccfa1..77d622d19e6 100644 --- a/gcc/analyzer/analyzer.cc +++ b/gcc/analyzer/analyzer.cc @@ -449,7 +449,7 @@ make_label_text (bool can_colorize, const char *fmt, ...) /* As above, but with singular vs plural. */ label_text -make_label_text_n (bool can_colorize, int n, +make_label_text_n (bool can_colorize, unsigned HOST_WIDE_INT n, const char *singular_fmt, const char *plural_fmt, ...) { diff --git a/gcc/analyzer/analyzer.h b/gcc/analyzer/analyzer.h index 35c71f3d69c..e0fdbad61a7 100644 --- a/gcc/analyzer/analyzer.h +++ b/gcc/analyzer/analyzer.h @@ -359,7 +359,8 @@ extern const char *get_user_facing_name (const gcall *call); extern void register_analyzer_pass (); extern label_text make_label_text (bool can_colorize, const char *fmt, ...); -extern label_text make_label_text_n (bool can_colorize, int n, +extern label_text make_label_text_n (bool can_colorize, + unsigned HOST_WIDE_INT n, const char *singular_fmt, const char *plural_fmt, ...); diff --git a/gcc/analyzer/bounds-checking.cc b/gcc/analyzer/bounds-checking.cc index 1c44790f86d..17f183fea21 100644 --- a/gcc/analyzer/bounds-checking.cc +++ b/gcc/analyzer/bounds-checking.cc @@ -32,6 +32,8 @@ along with GCC; see the file COPYING3. If not see #include "analyzer/analyzer.h" #include "analyzer/analyzer-logging.h" #include "analyzer/region-model.h" +#include "analyzer/checker-event.h" +#include "analyzer/checker-path.h" #if ENABLE_ANALYZER @@ -64,6 +66,20 @@ public: interest->add_region_creation (m_reg); } + void add_region_creation_events (const region *, + tree capacity, + location_t loc, + tree fndecl, int depth, + checker_path &emission_path) override + { + /* The memory space is described in the diagnostic message itself, + so we don't need an event for that. */ + if (capacity) + emission_path.add_event + (make_unique (capacity, + loc, fndecl, depth)); + } + protected: enum memory_space get_memory_space () const { @@ -147,14 +163,16 @@ public: other.m_byte_bound)); } - label_text - describe_region_creation_event (const evdesc::region_creation &ev) final - override + void add_region_creation_events (const region *, + tree, + location_t loc, + tree fndecl, int depth, + checker_path &emission_path) final override { if (m_byte_bound && TREE_CODE (m_byte_bound) == INTEGER_CST) - return ev.formatted_print ("capacity is %E bytes", m_byte_bound); - - return label_text (); + emission_path.add_event + (make_unique (m_byte_bound, + loc, fndecl, depth)); } protected: @@ -534,16 +552,6 @@ public: && pending_diagnostic::same_tree_p (m_capacity, other.m_capacity)); } - label_text - describe_region_creation_event (const evdesc::region_creation &ev) final - override - { - if (m_capacity) - return ev.formatted_print ("capacity is %qE bytes", m_capacity); - - return label_text (); - } - protected: tree m_offset; tree m_num_bytes; diff --git a/gcc/analyzer/checker-event.cc b/gcc/analyzer/checker-event.cc index a3e043333fe..98f1053da8f 100644 --- a/gcc/analyzer/checker-event.cc +++ b/gcc/analyzer/checker-event.cc @@ -293,84 +293,77 @@ statement_event::get_desc (bool) const /* class region_creation_event : public checker_event. */ -region_creation_event::region_creation_event (const region *reg, - tree capacity, - enum rce_kind kind, - location_t loc, +region_creation_event::region_creation_event (location_t loc, tree fndecl, int depth) -: checker_event (EK_REGION_CREATION, loc, fndecl, depth), - m_reg (reg), - m_capacity (capacity), - m_rce_kind (kind) +: checker_event (EK_REGION_CREATION, loc, fndecl, depth) { - if (m_rce_kind == RCE_CAPACITY) - gcc_assert (capacity); } -/* Implementation of diagnostic_event::get_desc vfunc for - region_creation_event. - There are effectively 3 kinds of region_region_event, to - avoid combinatorial explosion by trying to convy the - information in a single message. */ +/* The various region_creation_event subclasses' get_desc + implementations. */ label_text -region_creation_event::get_desc (bool can_colorize) const +region_creation_event_memory_space::get_desc (bool) const { - if (m_pending_diagnostic) + switch (m_mem_space) { - label_text custom_desc - = m_pending_diagnostic->describe_region_creation_event - (evdesc::region_creation (can_colorize, m_reg)); - if (custom_desc.get ()) - return custom_desc; + default: + return label_text::borrow ("region created here"); + case MEMSPACE_STACK: + return label_text::borrow ("region created on stack here"); + case MEMSPACE_HEAP: + return label_text::borrow ("region created on heap here"); } +} - switch (m_rce_kind) +label_text +region_creation_event_capacity::get_desc (bool can_colorize) const +{ + gcc_assert (m_capacity); + if (TREE_CODE (m_capacity) == INTEGER_CST) { - default: - gcc_unreachable (); - - case RCE_MEM_SPACE: - switch (m_reg->get_memory_space ()) - { - default: - return label_text::borrow ("region created here"); - case MEMSPACE_STACK: - return label_text::borrow ("region created on stack here"); - case MEMSPACE_HEAP: - return label_text::borrow ("region created on heap here"); - } - break; + unsigned HOST_WIDE_INT hwi = tree_to_uhwi (m_capacity); + return make_label_text_n (can_colorize, + hwi, + "capacity: %wu byte", + "capacity: %wu bytes", + hwi); + } + else + return make_label_text (can_colorize, + "capacity: %qE bytes", m_capacity); +} - case RCE_CAPACITY: - gcc_assert (m_capacity); +label_text +region_creation_event_allocation_size::get_desc (bool can_colorize) const +{ + if (m_capacity) + { if (TREE_CODE (m_capacity) == INTEGER_CST) - { - unsigned HOST_WIDE_INT hwi = tree_to_uhwi (m_capacity); - if (hwi == 1) - return make_label_text (can_colorize, - "capacity: %wu byte", hwi); - else - return make_label_text (can_colorize, - "capacity: %wu bytes", hwi); - } + return make_label_text_n (can_colorize, + tree_to_uhwi (m_capacity), + "allocated %E byte here", + "allocated %E bytes here", + m_capacity); else return make_label_text (can_colorize, - "capacity: %qE bytes", m_capacity); - - case RCE_DEBUG: - { - pretty_printer pp; - pp_format_decoder (&pp) = default_tree_printer; - pp_string (&pp, "region creation: "); - m_reg->dump_to_pp (&pp, true); - if (m_capacity) - pp_printf (&pp, " capacity: %qE", m_capacity); - return label_text::take (xstrdup (pp_formatted_text (&pp))); - } - break; + "allocated %qE bytes here", + m_capacity); } + return make_label_text (can_colorize, "allocated here"); +} + +label_text +region_creation_event_debug::get_desc (bool) const +{ + pretty_printer pp; + pp_format_decoder (&pp) = default_tree_printer; + pp_string (&pp, "region creation: "); + m_reg->dump_to_pp (&pp, true); + if (m_capacity) + pp_printf (&pp, " capacity: %qE", m_capacity); + return label_text::take (xstrdup (pp_formatted_text (&pp))); } /* class function_entry_event : public checker_event. */ diff --git a/gcc/analyzer/checker-event.h b/gcc/analyzer/checker-event.h index 18c44e600c8..f9885f5cfc3 100644 --- a/gcc/analyzer/checker-event.h +++ b/gcc/analyzer/checker-event.h @@ -22,6 +22,7 @@ along with GCC; see the file COPYING3. If not see #define GCC_ANALYZER_CHECKER_EVENT_H #include "tree-logical-location.h" +#include "analyzer/program-state.h" namespace ana { @@ -211,43 +212,105 @@ public: const program_state m_dst_state; }; -/* There are too many combinations to express region creation in one message, +/* An abstract event subclass describing the creation of a region that + is significant for a diagnostic. + + There are too many combinations to express region creation in one message, so we emit multiple region_creation_event instances when each pertinent region is created. - This enum distinguishes between the different messages. */ + The events are created by pending_diagnostic's add_region_creation_events + vfunc, which by default creates a region_creation_event_memory_space, and + if a capacity is known, a region_creation_event_capacity, giving e.g.: + (1) region created on stack here + (2) capacity: 100 bytes + but this vfunc can be overridden to create other events if other wordings + are more appropriate foa a given pending_diagnostic. */ -enum rce_kind +class region_creation_event : public checker_event { - /* Generate a message based on the memory space of the region - e.g. "region created on stack here". */ - RCE_MEM_SPACE, +protected: + region_creation_event (location_t loc, tree fndecl, int depth); +}; + +/* Concrete subclass of region_creation_event. + Generates a message based on the memory space of the region + e.g. "region created on stack here". */ - /* Generate a message based on the capacity of the region - e.g. "capacity: 100 bytes". */ - RCE_CAPACITY, +class region_creation_event_memory_space : public region_creation_event +{ +public: + region_creation_event_memory_space (enum memory_space mem_space, + location_t loc, tree fndecl, int depth) + : region_creation_event (loc, fndecl, depth), + m_mem_space (mem_space) + { + } - /* Generate a debug message. */ - RCE_DEBUG + label_text get_desc (bool can_colorize) const final override; + +private: + enum memory_space m_mem_space; }; -/* A concrete event subclass describing the creation of a region that - is significant for a diagnostic. */ +/* Concrete subclass of region_creation_event. + Generates a message based on the capacity of the region + e.g. "capacity: 100 bytes". */ -class region_creation_event : public checker_event +class region_creation_event_capacity : public region_creation_event { public: - region_creation_event (const region *reg, - tree capacity, - enum rce_kind kind, - location_t loc, tree fndecl, int depth); + region_creation_event_capacity (tree capacity, + location_t loc, tree fndecl, int depth) + : region_creation_event (loc, fndecl, depth), + m_capacity (capacity) + { + gcc_assert (m_capacity); + } + + label_text get_desc (bool can_colorize) const final override; + +private: + tree m_capacity; +}; + +/* Concrete subclass of region_creation_event. + Generates a message based on the capacity of the region + e.g. "allocated 100 bytes here". */ + +class region_creation_event_allocation_size : public region_creation_event +{ +public: + region_creation_event_allocation_size (tree capacity, + location_t loc, tree fndecl, int depth) + : region_creation_event (loc, fndecl, depth), + m_capacity (capacity) + {} + + label_text get_desc (bool can_colorize) const final override; + +private: + tree m_capacity; +}; + +/* Concrete subclass of region_creation_event. + Generates a debug message intended for analyzer developers. */ + +class region_creation_event_debug : public region_creation_event +{ +public: + region_creation_event_debug (const region *reg, tree capacity, + location_t loc, tree fndecl, int depth) + : region_creation_event (loc, fndecl, depth), + m_reg (reg), m_capacity (capacity) + { + } label_text get_desc (bool can_colorize) const final override; private: const region *m_reg; tree m_capacity; - enum rce_kind m_rce_kind; }; /* An event subclass describing the entry to a function. */ diff --git a/gcc/analyzer/checker-path.cc b/gcc/analyzer/checker-path.cc index 221042ee010..0cc0b2bf81f 100644 --- a/gcc/analyzer/checker-path.cc +++ b/gcc/analyzer/checker-path.cc @@ -141,7 +141,8 @@ checker_path::debug () const If DEBUG is true, also create an RCE_DEBUG event. */ void -checker_path::add_region_creation_events (const region *reg, +checker_path::add_region_creation_events (pending_diagnostic *pd, + const region *reg, const region_model *model, location_t loc, tree fndecl, int depth, @@ -152,16 +153,11 @@ checker_path::add_region_creation_events (const region *reg, if (const svalue *capacity_sval = model->get_capacity (reg)) capacity = model->get_representative_tree (capacity_sval); - add_event (make_unique (reg, capacity, RCE_MEM_SPACE, - loc, fndecl, depth)); - - if (capacity) - add_event (make_unique (reg, capacity, RCE_CAPACITY, - loc, fndecl, depth)); + pd->add_region_creation_events (reg, capacity, loc, fndecl, depth, *this); if (debug) - add_event (make_unique (reg, capacity, RCE_DEBUG, - loc, fndecl, depth)); + add_event (make_unique (reg, capacity, + loc, fndecl, depth)); } void diff --git a/gcc/analyzer/checker-path.h b/gcc/analyzer/checker-path.h index 55bf1e3e3b5..ba04aed069e 100644 --- a/gcc/analyzer/checker-path.h +++ b/gcc/analyzer/checker-path.h @@ -76,7 +76,8 @@ public: m_events[idx] = new_event; } - void add_region_creation_events (const region *reg, + void add_region_creation_events (pending_diagnostic *pd, + const region *reg, const region_model *model, location_t loc, tree fndecl, int depth, diff --git a/gcc/analyzer/diagnostic-manager.cc b/gcc/analyzer/diagnostic-manager.cc index 1b19e58201b..0574758be5a 100644 --- a/gcc/analyzer/diagnostic-manager.cc +++ b/gcc/analyzer/diagnostic-manager.cc @@ -1447,7 +1447,8 @@ diagnostic_manager::build_emission_path (const path_builder &pb, && DECL_SOURCE_LOCATION (decl) != UNKNOWN_LOCATION) { emission_path->add_region_creation_events - (reg, NULL, + (pb.get_pending_diagnostic (), + reg, NULL, DECL_SOURCE_LOCATION (decl), NULL_TREE, 0, @@ -1463,7 +1464,8 @@ diagnostic_manager::build_emission_path (const path_builder &pb, const exploded_edge *eedge = epath.m_edges[i]; add_events_for_eedge (pb, *eedge, emission_path, &interest); } - add_event_on_final_node (epath.get_final_enode (), emission_path, &interest); + add_event_on_final_node (pb, epath.get_final_enode (), + emission_path, &interest); } /* Emit a region_creation_event when requested on the last statement in @@ -1475,7 +1477,8 @@ diagnostic_manager::build_emission_path (const path_builder &pb, */ void -diagnostic_manager::add_event_on_final_node (const exploded_node *final_enode, +diagnostic_manager::add_event_on_final_node (const path_builder &pb, + const exploded_node *final_enode, checker_path *emission_path, interesting_t *interest) const { @@ -1512,7 +1515,8 @@ diagnostic_manager::add_event_on_final_node (const exploded_node *final_enode, case RK_HEAP_ALLOCATED: case RK_ALLOCA: emission_path->add_region_creation_events - (reg, + (pb.get_pending_diagnostic (), + reg, dst_model, src_point.get_location (), src_point.get_fndecl (), @@ -1940,7 +1944,8 @@ diagnostic_manager::add_events_for_eedge (const path_builder &pb, && DECL_SOURCE_LOCATION (decl) != UNKNOWN_LOCATION) { emission_path->add_region_creation_events - (reg, dst_state.m_region_model, + (pb.get_pending_diagnostic (), + reg, dst_state.m_region_model, DECL_SOURCE_LOCATION (decl), dst_point.get_fndecl (), dst_stack_depth, @@ -2036,7 +2041,8 @@ diagnostic_manager::add_events_for_eedge (const path_builder &pb, case RK_HEAP_ALLOCATED: case RK_ALLOCA: emission_path->add_region_creation_events - (reg, dst_model, + (pb.get_pending_diagnostic (), + reg, dst_model, src_point.get_location (), src_point.get_fndecl (), src_stack_depth, diff --git a/gcc/analyzer/diagnostic-manager.h b/gcc/analyzer/diagnostic-manager.h index 4862cf47241..56a233b9fa3 100644 --- a/gcc/analyzer/diagnostic-manager.h +++ b/gcc/analyzer/diagnostic-manager.h @@ -148,7 +148,8 @@ private: const exploded_path &epath, checker_path *emission_path) const; - void add_event_on_final_node (const exploded_node *final_enode, + void add_event_on_final_node (const path_builder &pb, + const exploded_node *final_enode, checker_path *emission_path, interesting_t *interest) const; diff --git a/gcc/analyzer/pending-diagnostic.cc b/gcc/analyzer/pending-diagnostic.cc index 53cab2065dd..babefc5ad4e 100644 --- a/gcc/analyzer/pending-diagnostic.cc +++ b/gcc/analyzer/pending-diagnostic.cc @@ -199,6 +199,26 @@ pending_diagnostic::add_call_event (const exploded_edge &eedge, src_stack_depth)); } +/* Base implementation of pending_diagnostic::add_region_creation_events. + See the comment for class region_creation_event. */ + +void +pending_diagnostic::add_region_creation_events (const region *reg, + tree capacity, + location_t loc, + tree fndecl, int depth, + checker_path &emission_path) +{ + emission_path.add_event + (make_unique (reg->get_memory_space (), + loc, fndecl, depth)); + + if (capacity) + emission_path.add_event + (make_unique (capacity, + loc, fndecl, depth)); +} + /* Base implementation of pending_diagnostic::add_final_event. Add a warning_event to the end of EMISSION_PATH. */ diff --git a/gcc/analyzer/pending-diagnostic.h b/gcc/analyzer/pending-diagnostic.h index 5d5d126b342..4bc3080c049 100644 --- a/gcc/analyzer/pending-diagnostic.h +++ b/gcc/analyzer/pending-diagnostic.h @@ -59,17 +59,6 @@ struct event_desc bool m_colorize; }; -/* For use by pending_diagnostic::describe_region_creation. */ - -struct region_creation : public event_desc -{ - region_creation (bool colorize, const region *reg) - : event_desc (colorize), m_reg (reg) - {} - - const region *m_reg; -}; - /* For use by pending_diagnostic::describe_state_change. */ struct state_change : public event_desc @@ -219,23 +208,6 @@ class pending_diagnostic for the diagnostic, and FALSE for events in their paths. */ virtual location_t fixup_location (location_t loc, bool primary) const; - /* For greatest precision-of-wording, the various following "describe_*" - virtual functions give the pending diagnostic a way to describe events - in a diagnostic_path in terms that make sense for that diagnostic. - - In each case, return a non-NULL label_text to give the event a custom - description; NULL otherwise (falling back on a more generic - description). */ - - /* Precision-of-wording vfunc for describing a region creation event - triggered by the mark_interesting_stuff vfunc. */ - virtual label_text - describe_region_creation_event (const evdesc::region_creation &) - { - /* Default no-op implementation. */ - return label_text (); - } - /* Precision-of-wording vfunc for describing a critical state change within the diagnostic_path. @@ -338,6 +310,15 @@ class pending_diagnostic virtual void add_call_event (const exploded_edge &, checker_path *); + /* Vfunc for adding any events for the creation of regions identified + by the mark_interesting_stuff vfunc. + See the comment for class region_creation_event. */ + virtual void add_region_creation_events (const region *reg, + tree capacity, + location_t loc, + tree fndecl, int depth, + checker_path &emission_path); + /* Vfunc for adding the final warning_event to a checker_path, so that e.g. the infinite recursion diagnostic can have its diagnostic appear at the callsite, but the final event in the path be at the entrypoint diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc index 4f623fd6ca3..c6486f315f4 100644 --- a/gcc/analyzer/region-model.cc +++ b/gcc/analyzer/region-model.cc @@ -74,6 +74,8 @@ along with GCC; see the file COPYING3. If not see #include "calls.h" #include "is-a.h" #include "gcc-rich-location.h" +#include "analyzer/checker-event.h" +#include "analyzer/checker-path.h" #if ENABLE_ANALYZER @@ -2777,12 +2779,14 @@ class dubious_allocation_size { public: dubious_allocation_size (const region *lhs, const region *rhs) - : m_lhs (lhs), m_rhs (rhs), m_expr (NULL_TREE) + : m_lhs (lhs), m_rhs (rhs), m_expr (NULL_TREE), + m_has_allocation_event (false) {} dubious_allocation_size (const region *lhs, const region *rhs, tree expr) - : m_lhs (lhs), m_rhs (rhs), m_expr (expr) + : m_lhs (lhs), m_rhs (rhs), m_expr (expr), + m_has_allocation_event (false) {} const char *get_kind () const final override @@ -2811,34 +2815,17 @@ public: " of the pointee's size"); } - label_text - describe_region_creation_event (const evdesc::region_creation &ev) final - override - { - m_allocation_event = &ev; - if (m_expr) - { - if (TREE_CODE (m_expr) == INTEGER_CST) - return ev.formatted_print ("allocated %E bytes here", m_expr); - else - return ev.formatted_print ("allocated %qE bytes here", m_expr); - } - - return ev.formatted_print ("allocated here"); - } - label_text describe_final_event (const evdesc::final_event &ev) final override { tree pointee_type = TREE_TYPE (m_lhs->get_type ()); - if (m_allocation_event) - /* Fallback: Typically, we should always - see an m_allocation_event before. */ + if (m_has_allocation_event) return ev.formatted_print ("assigned to %qT here;" " % is %qE", m_lhs->get_type (), pointee_type, size_in_bytes (pointee_type)); - + /* Fallback: Typically, we should always see an allocation_event + before. */ if (m_expr) { if (TREE_CODE (m_expr) == INTEGER_CST) @@ -2859,6 +2846,20 @@ public: size_in_bytes (pointee_type)); } + void + add_region_creation_events (const region *, + tree capacity, + location_t loc, + tree fndecl, int depth, + checker_path &emission_path) final override + { + emission_path.add_event + (make_unique (capacity, + loc, fndecl, depth)); + + m_has_allocation_event = true; + } + void mark_interesting_stuff (interesting_t *interest) final override { interest->add_region_creation (m_rhs); @@ -2868,7 +2869,7 @@ private: const region *m_lhs; const region *m_rhs; const tree m_expr; - const evdesc::region_creation *m_allocation_event; + bool m_has_allocation_event; }; /* Return true on dubious allocation sizes for constant sizes. */ diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c b/gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c index 235c156a25c..a56b25b4374 100644 --- a/gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-4.c @@ -56,6 +56,6 @@ void test_5 (void) free (ptr); /* { dg-warning "allocated buffer size is not a multiple of the pointee's size \\\[CWE-131\\\]" "warning" { target *-*-* } malloc5 } */ - /* { dg-message "1 bytes" "note" { target *-*-* } malloc5 } */ + /* { dg-message "allocated 1 byte here" "note" { target *-*-* } malloc5 } */ /* { dg-message "'struct base \\*' here; 'sizeof \\(struct base\\)' is '\\d+'" "note" { target *-*-* } malloc5 } */ } diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-multiline-1.c b/gcc/testsuite/gcc.dg/analyzer/allocation-size-multiline-1.c new file mode 100644 index 00000000000..7251665105d --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-multiline-1.c @@ -0,0 +1,59 @@ +/* { dg-additional-options "-fdiagnostics-path-format=inline-events -fdiagnostics-show-caret" } */ + +#include + +void test_constant_1 (void) +{ + int32_t *ptr = __builtin_malloc (1); /* { dg-warning "allocated buffer size is not a multiple of the pointee's size" } */ + __builtin_free (ptr); +} + +/* { dg-begin-multiline-output "" } + int32_t *ptr = __builtin_malloc (1); + ^~~~~~~~~~~~~~~~~~~~ + 'test_constant_1': events 1-2 + | + | int32_t *ptr = __builtin_malloc (1); + | ^~~~~~~~~~~~~~~~~~~~ + | | + | (1) allocated 1 byte here + | (2) assigned to 'int32_t *' + | + { dg-end-multiline-output "" } */ + +void test_constant_2 (void) +{ + int32_t *ptr = __builtin_malloc (2); /* { dg-warning "allocated buffer size is not a multiple of the pointee's size" } */ + __builtin_free (ptr); +} + +/* { dg-begin-multiline-output "" } + int32_t *ptr = __builtin_malloc (2); + ^~~~~~~~~~~~~~~~~~~~ + 'test_constant_2': events 1-2 + | + | int32_t *ptr = __builtin_malloc (2); + | ^~~~~~~~~~~~~~~~~~~~ + | | + | (1) allocated 2 bytes here + | (2) assigned to 'int32_t *' + | + { dg-end-multiline-output "" } */ + +void test_symbolic (int n) +{ + int32_t *ptr = __builtin_malloc (n * 2); /* { dg-warning "allocated buffer size is not a multiple of the pointee's size" } */ + __builtin_free (ptr); +} + +/* { dg-begin-multiline-output "" } + int32_t *ptr = __builtin_malloc (n * 2); + ^~~~~~~~~~~~~~~~~~~~~~~~ + 'test_symbolic': event 1 + | + | int32_t *ptr = __builtin_malloc (n * 2); + | ^~~~~~~~~~~~~~~~~~~~~~~~ + | | + | (1) allocated 'n * 2' bytes and assigned to 'int32_t *' + | + { dg-end-multiline-output "" } */ diff --git a/gcc/testsuite/gcc.dg/analyzer/allocation-size-multiline-2.c b/gcc/testsuite/gcc.dg/analyzer/allocation-size-multiline-2.c new file mode 100644 index 00000000000..7cadbb74751 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/allocation-size-multiline-2.c @@ -0,0 +1,62 @@ +/* { dg-additional-options "-fdiagnostics-path-format=inline-events -fdiagnostics-show-caret -fanalyzer-fine-grained" } */ +/* { dg-require-effective-target alloca } */ + +#include + +void test_constant_1 (void) +{ + int32_t *ptr = __builtin_alloca (1); /* { dg-warning "allocated buffer size is not a multiple of the pointee's size" } */ +} + +/* { dg-begin-multiline-output "" } + int32_t *ptr = __builtin_alloca (1); + ^~~~~~~~~~~~~~~~~~~~ + 'test_constant_1': events 1-2 + | + | int32_t *ptr = __builtin_alloca (1); + | ^~~~~~~~~~~~~~~~~~~~ + | | + | (1) allocated 1 byte here + | (2) assigned to 'int32_t *' + | + { dg-end-multiline-output "" } */ + +void test_constant_2 (void) +{ + int32_t *ptr = __builtin_alloca (2); /* { dg-warning "allocated buffer size is not a multiple of the pointee's size" } */ +} + +/* { dg-begin-multiline-output "" } + int32_t *ptr = __builtin_alloca (2); + ^~~~~~~~~~~~~~~~~~~~ + 'test_constant_2': events 1-2 + | + | int32_t *ptr = __builtin_alloca (2); + | ^~~~~~~~~~~~~~~~~~~~ + | | + | (1) allocated 2 bytes here + | (2) assigned to 'int32_t *' + | + { dg-end-multiline-output "" } */ + +void test_symbolic (int n) +{ + int32_t *ptr = __builtin_alloca (n * 2); /* { dg-warning "allocated buffer size is not a multiple of the pointee's size" } */ +} + +/* { dg-begin-multiline-output "" } + int32_t *ptr = __builtin_alloca (n * 2); + ^~~~~~~~~~~~~~~~~~~~~~~~ + 'test_symbolic': events 1-2 + | + | int32_t *ptr = __builtin_alloca (n * 2); + | ^~~~~~~~~~~~~~~~~~~~~~~~ + | | + | (1) allocated 'n * 2' bytes here + | (2) assigned to 'int32_t *' + | + { dg-end-multiline-output "" } */ + +/* FIXME: am getting a duplicate warning here for some reason + without -fanalyzer-fine-grained (PR PR analyzer/107851). */ + diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-multiline-1.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-multiline-1.c index 25301e9e2ff..ca5022d20cb 100644 --- a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-multiline-1.c +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-multiline-1.c @@ -25,7 +25,7 @@ void int_arr_write_element_after_end_off_by_one(int32_t x) | int32_t arr[10]; | ^~~ | | - | (1) capacity is 40 bytes + | (1) capacity: 40 bytes | +--> 'int_arr_write_element_after_end_off_by_one': event 2 (depth 1) | diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-multiline-2.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-multiline-2.c new file mode 100644 index 00000000000..660901ab782 --- /dev/null +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-multiline-2.c @@ -0,0 +1,32 @@ +/* Integration test of how the execution path looks for + -Wanalyzer-out-of-bounds with a symbolic size. */ + +/* { dg-additional-options "-fdiagnostics-show-path-depths" } */ +/* { dg-additional-options "-fdiagnostics-path-format=inline-events -fdiagnostics-show-caret" } */ + +#include +#include + +void int_vla_write_element_after_end_off_by_one(int32_t x, size_t n) +{ + int32_t arr[n]; + + arr[n] = x; /* { dg-warning "stack-based buffer overflow" } */ +} + +/* { dg-begin-multiline-output "" } + arr[n] = x; + ~~~~~~~^~~ + 'int_vla_write_element_after_end_off_by_one': events 1-2 (depth 1) + | + | int32_t arr[n]; + | ^~~ + | | + | (1) capacity: 'n * 4' bytes + | + | arr[n] = x; + | ~~~~~~~~~~ + | | + | (2) write of 4 bytes at offset 'n * 4' exceeds the buffer + | + { dg-end-multiline-output "" } */ diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-read-char-arr.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-read-char-arr.c index f6d0dda9fb9..fa4b613550d 100644 --- a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-read-char-arr.c +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-read-char-arr.c @@ -1,4 +1,4 @@ -char arr[10]; /* { dg-message "capacity is 10 bytes" } */ +char arr[10]; /* { dg-message "capacity: 10 bytes" } */ char char_arr_read_element_before_start_far(void) { diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-read-int-arr.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-read-int-arr.c index f1b6e119777..c04cc19df93 100644 --- a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-read-int-arr.c +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-read-int-arr.c @@ -1,6 +1,6 @@ #include -int32_t arr[10]; /* { dg-message "capacity is 40 bytes" } */ +int32_t arr[10]; /* { dg-message "capacity: 40 bytes" } */ int32_t int_arr_read_element_before_start_far(void) { diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-write-char-arr.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-write-char-arr.c index 2f3bbfa2dc2..2bc707c8d03 100644 --- a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-write-char-arr.c +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-write-char-arr.c @@ -1,4 +1,4 @@ -char arr[10]; /* { dg-message "capacity is 10 bytes" } */ +char arr[10]; /* { dg-message "capacity: 10 bytes" } */ void char_arr_write_element_before_start_far(char x) { diff --git a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-write-int-arr.c b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-write-int-arr.c index 0adb7899641..c6c0435cf58 100644 --- a/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-write-int-arr.c +++ b/gcc/testsuite/gcc.dg/analyzer/out-of-bounds-write-int-arr.c @@ -1,6 +1,6 @@ #include -int32_t arr[10]; /* { dg-message "capacity is 40 bytes" } */ +int32_t arr[10]; /* { dg-message "capacity: 40 bytes" } */ void int_arr_write_element_before_start_far(int32_t x) {