From patchwork Fri Oct 4 22:36:21 2024 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: David Malcolm X-Patchwork-Id: 1992988 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=K0SIw54w; dkim-atps=neutral Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=2620:52:3:1:0:246e:9693:128c; helo=server2.sourceware.org; envelope-from=gcc-patches-bounces~incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=patchwork.ozlabs.org) Received: from server2.sourceware.org (server2.sourceware.org [IPv6:2620:52:3:1:0:246e:9693:128c]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (secp384r1) server-digest SHA384) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4XL3RL6S1Vz1xsn for ; Sat, 5 Oct 2024 08:40:43 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 031BF385EC3D for ; Fri, 4 Oct 2024 22:40:41 +0000 (GMT) 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 ESMTP id 48CBF385E003 for ; Fri, 4 Oct 2024 22:36:28 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 48CBF385E003 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 48CBF385E003 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1728081472; cv=none; b=Fw5bN/BGNSfrVFd7xzbTPCueRG0y34RfzW5fsPZw5QJTrfvhBYQse7GHoad9Rcqt4+frdDoIOef+oTOCFPudq/+GO6viRuKgrLSC8Bh2EVwmYJOc5TWCn+NncIOFZ/lNXLaJuHRIipAxGcAw9RGfPZsUJCvhkBlVsWxABqOly3M= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1728081472; c=relaxed/simple; bh=5BUMbaxPsLy9bKhkDUgKAFPyTO+LTM2AAOqpV/HFKao=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=k5NJzVcp9iflicFiunrhc9pmA8BcDEn1of+5yoE2T5qVBA7xrCaB1fG8vQqH2Z1p5y1ClqTuY1D/anoL1l0tDyPyz+RaUjHtgw7AvhAR5MzOCNMOv3zhyICIdijAFOSRPsznIyF2n/Tf0WK9laRRYvvLHfWJT6YWVh1ndRR1PaQ= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1728081387; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=+x7GvZbkT1upgpLkgFFuY7YXClEsVPyXpLxDX+ptkUU=; b=K0SIw54w8LgSNWTdEXEysUyl9VzDPSn99PQLe/HitHb/efOxcrTqAJmVghRsOiG8EDlXGr CuTI9qIblECTAjKeia72062UKvJ9qOFedSt8JPM9TseIDcwsKB1PiaRQgdRP6n2l2O3/qp ZRXDUdz2d1YPS/sa1pii4x6T5UDi8Go= Received: from mx-prod-mc-01.mail-002.prod.us-west-2.aws.redhat.com (ec2-54-186-198-63.us-west-2.compute.amazonaws.com [54.186.198.63]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-549-rRi41Gc6OCSA-5A7ki7lLA-1; Fri, 04 Oct 2024 18:36:26 -0400 X-MC-Unique: rRi41Gc6OCSA-5A7ki7lLA-1 Received: from mx-prod-int-05.mail-002.prod.us-west-2.aws.redhat.com (mx-prod-int-05.mail-002.prod.us-west-2.aws.redhat.com [10.30.177.17]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mx-prod-mc-01.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTPS id AA8D91955BF4 for ; Fri, 4 Oct 2024 22:36:25 +0000 (UTC) Received: from t14s.localdomain.com (unknown [10.22.8.67]) by mx-prod-int-05.mail-002.prod.us-west-2.aws.redhat.com (Postfix) with ESMTP id 11DCD1955E93; Fri, 4 Oct 2024 22:36:23 +0000 (UTC) From: David Malcolm To: gcc-patches@gcc.gnu.org Cc: David Malcolm Subject: [pushed: r15-4081] diagnostics: bulletproof opening of SARIF output [PR116978] Date: Fri, 4 Oct 2024 18:36:21 -0400 Message-ID: <20241004223621.3400595-1-dmalcolm@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.0 on 10.30.177.17 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com X-Spam-Status: No, score=-11.7 required=5.0 tests=BAYES_00, DKIMWL_WL_HIGH, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, GIT_PATCH_0, KAM_SHORT, RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H3, RCVD_IN_MSPIKE_WL, 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.30 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces~incoming=patchwork.ozlabs.org@gcc.gnu.org Introduce a new RAII class diagnostic_output_file to track ownership of the FILE * for SARIF output. In particular, the .sarif file is now opened immediately, rather than at the end of the compile, and so will fail earlier if the file can't be opened. Doing so fixes a couple of ICEs in -fdiagnostics-format=sarif-file when invoking, say, cc1 directly, rather than from the driver. Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu. Pushed to trunk as r15-4081-g385a232229a5b4. gcc/ChangeLog: PR other/116978 * diagnostic-format-sarif.cc (sarif_builder::sarif_builder): Gracefully handle "main_input_filename_" being NULL. (sarif_output_format::sarif_output_format): Replace param "base_file_name" with "output_file" and assert that the file was opened successfully and has a non-NULL filename. (sarif_output_format::~sarif_file_output_format): Move responsibility for building the filename and opening the file from here to the creator of the instance. (sarif_output_format::m_base_file_name): Replace with... (sarif_output_format::m_output_file): ...this. (diagnostic_output_format_init_sarif_file): Make "line_maps" param non-const. Gracefully handle "base_file_name" being NULL. Construct the filename and open the file here, rather than in ~sarif_file_output_format, and handle failures immediately here, rather than at the end of the compile. * diagnostic-format-sarif.h: Include "diagnostic-output-file.h". (diagnostic_output_format_init_sarif_file): Make "line_maps" param non-const. * diagnostic-output-file.h: New file. * diagnostic.cc (diagnostic_context::emit_diagnostic): New. (diagnostic_context::emit_diagnostic_va): New. * diagnostic.h (diagnostic_context::emit_diagnostic): New decl. (diagnostic_context::emit_diagnostic_va): New decl. Signed-off-by: David Malcolm --- gcc/diagnostic-format-sarif.cc | 57 +++++++++++++++----------- gcc/diagnostic-format-sarif.h | 3 +- gcc/diagnostic-output-file.h | 75 ++++++++++++++++++++++++++++++++++ gcc/diagnostic.cc | 41 +++++++++++++++++++ gcc/diagnostic.h | 13 ++++++ 5 files changed, 165 insertions(+), 24 deletions(-) create mode 100644 gcc/diagnostic-output-file.h diff --git a/gcc/diagnostic-format-sarif.cc b/gcc/diagnostic-format-sarif.cc index aeba9f9ceef3..f053dec19205 100644 --- a/gcc/diagnostic-format-sarif.cc +++ b/gcc/diagnostic-format-sarif.cc @@ -1510,9 +1510,10 @@ sarif_builder::sarif_builder (diagnostic_context &context, since otherwise the "no diagnostics" case would quote the main input file, and doing so noticeably bloated the output seen in analyzer integration testing (build directory went from 20G -> 21G). */ - get_or_create_artifact (main_input_filename_, - diagnostic_artifact_role::analysis_target, - false); + if (main_input_filename_) + get_or_create_artifact (main_input_filename_, + diagnostic_artifact_role::analysis_target, + false); } sarif_builder::~sarif_builder () @@ -3239,29 +3240,17 @@ public: const char *main_input_filename_, bool formatted, enum sarif_version version, - const char *base_file_name) + diagnostic_output_file output_file) : sarif_output_format (context, line_maps, main_input_filename_, formatted, version), - m_base_file_name (xstrdup (base_file_name)) + m_output_file (std::move (output_file)) { + gcc_assert (m_output_file.get_open_file ()); + gcc_assert (m_output_file.get_filename ()); } ~sarif_file_output_format () { - char *filename = concat (m_base_file_name, ".sarif", nullptr); - free (m_base_file_name); - m_base_file_name = nullptr; - FILE *outf = fopen (filename, "w"); - if (!outf) - { - const char *errstr = xstrerror (errno); - fnotice (stderr, "error: unable to open '%s' for writing: %s\n", - filename, errstr); - free (filename); - return; - } - m_builder.flush_to_file (outf); - fclose (outf); - free (filename); + m_builder.flush_to_file (m_output_file.get_open_file ()); } bool machine_readable_stderr_p () const final override { @@ -3269,7 +3258,7 @@ public: } private: - char *m_base_file_name; + diagnostic_output_file m_output_file; }; /* Print the start of an embedded link to PP, as per 3.11.6. */ @@ -3435,13 +3424,35 @@ diagnostic_output_format_init_sarif_stderr (diagnostic_context &context, void diagnostic_output_format_init_sarif_file (diagnostic_context &context, - const line_maps *line_maps, + line_maps *line_maps, const char *main_input_filename_, bool formatted, enum sarif_version version, const char *base_file_name) { gcc_assert (line_maps); + + if (!base_file_name) + { + rich_location richloc (line_maps, UNKNOWN_LOCATION); + context.emit_diagnostic (DK_ERROR, richloc, nullptr, 0, + "unable to determine filename for SARIF output"); + return; + } + + label_text filename = label_text::take (concat (base_file_name, + ".sarif", + nullptr)); + FILE *outf = fopen (filename.get (), "w"); + if (!outf) + { + rich_location richloc (line_maps, UNKNOWN_LOCATION); + context.emit_diagnostic (DK_ERROR, richloc, nullptr, 0, + "unable to open %qs for SARIF output: %m", + filename.get ()); + return; + } + diagnostic_output_file output_file (outf, true, std::move (filename)); diagnostic_output_format_init_sarif (context, ::make_unique (context, @@ -3449,7 +3460,7 @@ diagnostic_output_format_init_sarif_file (diagnostic_context &context, main_input_filename_, formatted, version, - base_file_name)); + std::move (output_file))); } /* Populate CONTEXT in preparation for SARIF output to STREAM. */ diff --git a/gcc/diagnostic-format-sarif.h b/gcc/diagnostic-format-sarif.h index 555ea60a70b4..5f8751aa3505 100644 --- a/gcc/diagnostic-format-sarif.h +++ b/gcc/diagnostic-format-sarif.h @@ -23,6 +23,7 @@ along with GCC; see the file COPYING3. If not see #include "json.h" #include "diagnostic-format.h" +#include "diagnostic-output-file.h" class logical_location; @@ -42,7 +43,7 @@ diagnostic_output_format_init_sarif_stderr (diagnostic_context &context, enum sarif_version version); extern void diagnostic_output_format_init_sarif_file (diagnostic_context &context, - const line_maps *line_maps, + line_maps *line_maps, const char *main_input_filename_, bool formatted, enum sarif_version version, diff --git a/gcc/diagnostic-output-file.h b/gcc/diagnostic-output-file.h new file mode 100644 index 000000000000..f0ae5e1915ec --- /dev/null +++ b/gcc/diagnostic-output-file.h @@ -0,0 +1,75 @@ +/* RAII class for managing FILE * for diagnostic formats. + Copyright (C) 2024 Free Software Foundation, Inc. + Contributed by David Malcolm . + +This file is part of GCC. + +GCC is free software; you can redistribute it and/or modify it under +the terms of the GNU General Public License as published by the Free +Software Foundation; either version 3, or (at your option) any later +version. + +GCC is distributed in the hope that it will be useful, but WITHOUT ANY +WARRANTY; without even the implied warranty of MERCHANTABILITY or +FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License +for more details. + +You should have received a copy of the GNU General Public License +along with GCC; see the file COPYING3. If not see +. */ + +#ifndef GCC_DIAGNOSTIC_OUTPUT_FILE_H +#define GCC_DIAGNOSTIC_OUTPUT_FILE_H + +/* RAII class for wrapping a FILE * that could be borrowed or owned, + along with the underlying filename. */ + +class diagnostic_output_file +{ +public: + diagnostic_output_file (FILE *outf, bool owned, label_text filename) + : m_outf (outf), + m_owned (owned), + m_filename (std::move (filename)) + { + gcc_assert (m_filename.get ()); + if (m_owned) + gcc_assert (m_outf); + } + ~diagnostic_output_file () + { + if (m_owned) + { + gcc_assert (m_outf); + fclose (m_outf); + } + } + diagnostic_output_file (const diagnostic_output_file &other) = delete; + diagnostic_output_file (diagnostic_output_file &&other) + : m_outf (other.m_outf), + m_owned (other.m_owned), + m_filename (std::move (other.m_filename)) + { + other.m_outf = nullptr; + other.m_owned = false; + + gcc_assert (m_filename.get ()); + if (m_owned) + gcc_assert (m_outf); + } + diagnostic_output_file & + operator= (const diagnostic_output_file &other) = delete; + diagnostic_output_file & + operator= (diagnostic_output_file &&other) = delete; + + operator bool () const { return m_outf != nullptr; } + FILE *get_open_file () const { return m_outf; } + const char *get_filename () const { return m_filename.get (); } + +private: + FILE *m_outf; + bool m_owned; + label_text m_filename; +}; + +#endif /* ! GCC_DIAGNOSTIC_OUTPUT_FILE_H */ diff --git a/gcc/diagnostic.cc b/gcc/diagnostic.cc index dfedcae58052..c8449d82d185 100644 --- a/gcc/diagnostic.cc +++ b/gcc/diagnostic.cc @@ -1178,6 +1178,47 @@ diagnostic_context::warning_enabled_at (location_t loc, return diagnostic_enabled (&diagnostic); } +/* Emit a diagnostic within a diagnostic group on this context. */ + +bool +diagnostic_context::emit_diagnostic (diagnostic_t kind, + rich_location &richloc, + const diagnostic_metadata *metadata, + diagnostic_option_id option_id, + const char *gmsgid, ...) +{ + begin_group (); + + va_list ap; + va_start (ap, gmsgid); + bool ret = emit_diagnostic_va (kind, richloc, metadata, option_id, + gmsgid, &ap); + va_end (ap); + + end_group (); + + return ret; +} + +/* As above, but taking a va_list *. */ + +bool +diagnostic_context::emit_diagnostic_va (diagnostic_t kind, + rich_location &richloc, + const diagnostic_metadata *metadata, + diagnostic_option_id option_id, + const char *gmsgid, va_list *ap) +{ + begin_group (); + + bool ret = diagnostic_impl (&richloc, metadata, option_id, + gmsgid, ap, kind); + + end_group (); + + return ret; +} + /* Report a diagnostic message (an error or a warning) as specified by this diagnostic_context. front-end independent format specifiers are exactly those described diff --git a/gcc/diagnostic.h b/gcc/diagnostic.h index fd056bd87225..9c11ee3d1cfe 100644 --- a/gcc/diagnostic.h +++ b/gcc/diagnostic.h @@ -506,6 +506,19 @@ public: return m_option_classifier.option_unspecified_p (option_id); } + bool emit_diagnostic (diagnostic_t kind, + rich_location &richloc, + const diagnostic_metadata *metadata, + diagnostic_option_id option_id, + const char *gmsgid, ...) + ATTRIBUTE_GCC_DIAG(6,7); + bool emit_diagnostic_va (diagnostic_t kind, + rich_location &richloc, + const diagnostic_metadata *metadata, + diagnostic_option_id option_id, + const char *gmsgid, va_list *ap) + ATTRIBUTE_GCC_DIAG(6,0); + bool report_diagnostic (diagnostic_info *); void check_max_errors (bool flush);