On Fri, Nov 04, 2022 at 12:42:29PM -0400, David Malcolm wrote:
> On Fri, 2022-11-04 at 09:44 -0400, Lewis Hyatt via Gcc-patches wrote:
> > Add awareness that diagnostic locations may be in generated buffers
> > rather
> > than an actual file to other places in the diagnostics code that may
> > care,
> > most notably SARIF output (which needs to obtain its own snapshots of
> > the code
> > involved). For edit context output, which outputs fixit hints as
> > diffs, for
> > now just make sure we ignore generated data buffers. At the moment,
> > there is
> > no ability for a fixit hint to be generated in such a buffer.
> > 
> > Because SARIF uses JSON as well, also add the ability to the
> > json::string
> > class to handle a buffer with nulls in the middle (since we place no
> > restriction on LC_GEN content) by providing the option to specify the
> > data
> > length.
> 
> Please can you split this patch into three parts:
> - the SARIF part
> - the json changes
> - the edit-context.cc changes (I think this at least counts as an
> "obvious" change with respect to the other changes in the kit, though
> I'm still working my way through patch 4 in the kit).
> 
> Please add a DejaGnu testcase to the SARIF part, with a diagnostic that
> references a generated data buffer; see
>   gcc/testsuite/c-c++-common/diagnostic-format-sarif-file-*.c 
> for examples of SARIF testcases.
> 
> Please add a selftest to the json change so that we have a unit test of
> constructing a json::string with an embedded NUL, and how we serialize
> such a string (probably to json.cc's test_writing_strings)
> 
> Thanks
> Dave

Yes, certainly, sorry for not splitting it up more to start with. Regarding
the SARIF testcase, it's not that easy to get SARIF output to actually output
generated data, because as of now it can only appear in a _Pragma, and SARIF
does not output macro definitions currently. I think the only way I know to do
it, is to make use of -fdump-internal-locations, which generates top-level
inform() calls inside the _Pragma that can end up in the SARIF output. So I
wrote a testcase that does this, but not sure how you will feel about having
the testsuite rely on this internal debugging option.

I wasn't sure what's the best way to send the 3 split up patches. I attached
them here as 5a/6, 5b/6, 5c/6, in case that's right, but I wasn't sure if I should just
resend the whole batch (minus perhaps the 2 you have already acked), and/or if
I should wait for feedback on the other patches first. Happy to do whatever
makes it easier for you, and thanks for your time! Note that the new SARIF
patch (5c/6) now needs to come last in the series, after the patch 6/6 that
actually supports _Pragma, so that the new testcase can make use of that.

-Lewis