From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: 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 CA70E3858D3C for ; Tue, 7 Nov 2023 14:51:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org CA70E3858D3C 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 CA70E3858D3C 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=1699368688; cv=none; b=C1Mi9pe/YQWKQGNJLkVZNjpfyxKALbY8ncOOa5ixthol9hEBKBnOna0zpkQWDIh+2oc+PKScpTfocjL0xj1F1FWvcnz40HVplg4/80SaPzvki9083EVqVgwOIu152zQJS3VuQUMWJPYcScece8wCeAZw8jNchREKXdMX59B6igg= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1699368688; c=relaxed/simple; bh=+gAptUaFhvGAatQGbTrreS5U8L8Loqu5rUuTrG/yUS8=; h=DKIM-Signature:Message-ID:Subject:From:To:Date:MIME-Version; b=eXHpX9kYf+zQw7yuALmcKiRBFZJuhmcWkFj2nkJxSJZdpzEqDU1t/13yJ5Rdm+AKoQjrNvqwQSH/nRs31yFTuV8bvbNVH9Q+4MQEN/WcNWW7PcY/Zn9PwmzmsFZeJ1AK9S9jrwFpAbjblmzfPmO6JYFiY9OgRA1Ndj5xLRJ89Ww= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1699368686; 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: in-reply-to:in-reply-to:references:references; bh=d83J+CU/a0kOwa/4FL1lHzIDfufMA6Quxvl5Tc+j+Zs=; b=eONt4+eQyToDFLMJBHvO/GYehT1rn9iaVbN57otKQMBG44h1G/EHhoFQ4UVkImz/8a9G0O FX0ZhTycpK5IYSJ7EX6Fz4YE4YzdeKvvZcEzILWdhYMit3yrP8St5h2D4wKK/CoqlaN/k1 QZPlhqrohx0vDWcAf+pj3p8g8l/oM2s= Received: from mail-qt1-f198.google.com (mail-qt1-f198.google.com [209.85.160.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-449-YgavCON6MDmIoV2VtNZnrg-1; Tue, 07 Nov 2023 09:51:24 -0500 X-MC-Unique: YgavCON6MDmIoV2VtNZnrg-1 Received: by mail-qt1-f198.google.com with SMTP id d75a77b69052e-41cb6df5c7bso61287461cf.3 for ; Tue, 07 Nov 2023 06:51:23 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1699368683; x=1699973483; h=mime-version:user-agent:content-transfer-encoding:references :in-reply-to:date:cc:to:from:subject:message-id:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=d83J+CU/a0kOwa/4FL1lHzIDfufMA6Quxvl5Tc+j+Zs=; b=V9JhYXA543mUXUHLSo1OgJHHJG0I4ax4U79Jxb22pp4zVtu/tzE73ZODXGW2Bp6s59 d6n9CX5JJsbQyti1GOxpCSbygtEGJ34nylSnURjlnHdtdB+d/ofKxl/v6jUMXF7I25NQ PHHp0k/LXmzc9EMcWT/oDx6565JcJISNrI6Lgfzs+QvuHDk/YFuNBYEFMvBm//GjS08p vNKdO09KOLE+wTaBPPHYt3UyJW0E2jXTODiGwbj35qzDhq1oRvZvbxTj13hxdTmGjzAQ +CoH33sg54UhD3APeFogCyYe/rogHx7fIwehUVSMk/zzFGw7B30zqKEqXHw1R6aYEYaV oNQQ== X-Gm-Message-State: AOJu0YzgOT5M15YkoRIIBXSWVXGVfBLlqjELq4OENKX9CkAj5yZIuMN0 gFzvs+eF20M99xSG2E843PCnKbPztFCzpHTx/Kits3e5yVh2024wqnfHJgUWUffWGCyU9Qss7n0 GwPflgQb962+esDn4qYrxZdvNAmfz X-Received: by 2002:ac8:5fd1:0:b0:420:5b48:4a54 with SMTP id k17-20020ac85fd1000000b004205b484a54mr1234121qta.23.1699368683121; Tue, 07 Nov 2023 06:51:23 -0800 (PST) X-Google-Smtp-Source: AGHT+IHaD/9hOcRekBg7pb6pqf5nI+0izF9fHrXYFQF/qlgjnmiIrwzlA72w6PiwEb3i2Ve+zbZBkw== X-Received: by 2002:ac8:5fd1:0:b0:420:5b48:4a54 with SMTP id k17-20020ac85fd1000000b004205b484a54mr1234104qta.23.1699368682804; Tue, 07 Nov 2023 06:51:22 -0800 (PST) Received: from t14s.localdomain (c-76-28-97-5.hsd1.ma.comcast.net. [76.28.97.5]) by smtp.gmail.com with ESMTPSA id i5-20020ac813c5000000b0041b9b6eb309sm4334314qtj.93.2023.11.07.06.51.22 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 07 Nov 2023 06:51:22 -0800 (PST) Message-ID: <2dc5f980e6fb9c7d30d46debefd1cf062458ca36.camel@redhat.com> Subject: Re: [PATCH] binutils: experimental use of libdiagnostics in gas From: David Malcolm To: Simon Sobisch , gcc-patches@gcc.gnu.org, binutils@sourceware.org Cc: Nick Clifton Date: Tue, 07 Nov 2023 09:51:21 -0500 In-Reply-To: References: <20231106222959.2707741-1-dmalcolm@redhat.com> <20231106222959.2707741-4-dmalcolm@redhat.com> User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-5.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,KAM_SHORT,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_NONE,TXREP,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Tue, 2023-11-07 at 08:04 +0100, Simon Sobisch wrote: > Thank you very much for this proof-of-concept use! >=20 > Inspecting it raises the following questions to me, both for a > possible=20 > binutils implementation and for the library use in general: >=20 > * How should the application set the relevant context (often lines > are > =C2=A0=C2=A0 shown before/after)? Currently the source-printing code has this logic (in gcc/diagnostic- show-locus.cc): - filter locations within the diagnostic to "sufficiently sane" ones (thus ignoring e.g. ranges that end before they start) - look at all of the remaining locations that are in the same source file as the primary location of the diagnostic - determine a set of runs of source lines; layout::calculate_line_spans has this comment: /* We want to print the pertinent source code at a diagnostic. The rich_location can contain multiple locations. This will have been filtered into m_exploc (the caret for the primary location) and m_layout_ranges, for those ranges within the same source file. We will print a subset of the lines within the source file in question, as a collection of "spans" of lines. This function populates m_line_spans with an ordered, disjoint list of the line spans of interest. Printing a gap between line spans takes one line, so, when printing line numbers, we allow a gap of up to one line between spans when merging, since it makes more sense to print the source line rather than = a "gap-in-line-numbering" line. When not printing line numbers, it's better to be more explicit about what's going on, so keeping them as separate spans is preferred. For example, if the primary range is on lines 8-10, with secondary range= s covering lines 5-6 and lines 13-15: 004 005 |RANGE 1 006 |RANGE 1 007 008 |PRIMARY RANGE 009 |PRIMARY CARET 010 |PRIMARY RANGE 011 012 013 |RANGE 2 014 |RANGE 2 015 |RANGE 2 016 With line numbering on, we want two spans: lines 5-10 and lines 13-15. With line numbering off (with span headers), we want three spans: lines = 5-6, lines 8-10, and lines 13-15. */ (end of quote) This algorithm could be tweaked if you want extra lines of context, perhaps having an integer option N for N extra lines of before/after context around each run. > * Should it be possible to override msgid used to display the > =C2=A0=C2=A0 warning/error type? > =C2=A0=C2=A0 If this would be possible then the text sink in messages_ini= t may > be > =C2=A0=C2=A0 adjusted to replace the label with _("Warning") and _("Error= "), > which > =C2=A0=C2=A0 would leave the text output "as-is" (if the text sink is > configured to > =C2=A0=C2=A0 not output the source line); this would make it usable witho= ut > =C2=A0=C2=A0 adjusting the testsuite and to adopt to a standard later. For the msgids, I was more thinking of the messages of the diagnostics themselves; for instance, in GCC the error format string: "Invalid argument %d for builtin %qF" has fr.po translation: "Argument %d invalide pour la fonction interne %qF" But it sounds like gas also has capitalized severities (e.g. "Warning"), so maybe that should simply be a flag in the text sink. >=20 >=20 > Notes for the SARIF output: > * the region contains an error, according to the linked JSON spec > =C2=A0=C2=A0 startColumn has a minimum of 1 (I guess you'd just leave it = away > if > =C2=A0=C2=A0 the application did not set it) Good catch; looks like a bug in my SARIF output code. I've filed it for myself as: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D112425 > * the application should have the option to pre-set the > sourceLanguage > =C2=A0=C2=A0 for the diagnostic_manager (maybe even make that a positiona= l > argument > =C2=A0=C2=A0 that needs to be passed but can be NULL) and override it whe= n > =C2=A0=C2=A0 specifying a region Why? Note that the sourceLanguage can always be NULL. I considered setting it for gas, but it's not clear what the value can be, so I just omit it; see: https://github.com/oasis-tcs/sarif-spec/issues/608 [..snip...] Thanks for the feedback Dave