On Thu, 2026-03-12 at 08:45 +0100, Tomás Ortín Fernández wrote:
> Thank you for the review.
>
> > > +class kf_mkstemp : public known_function
> > > +{
> > > +public:
> > > + bool
> > > + matches_call_types_p (const call_details &cd) const final
> > > override
> > > + {
> > > + return (cd.num_args () == 1 && cd.arg_is_pointer_p (0));
> > > + }
> > > +
> > > + void
> > > + impl_call_pre (const call_details &cd) const final override
> > > + {
> > > + region_model_context *ctxt = cd.get_ctxt ();
> > > + /* If there's no context we can't store warnings, so we
> > > return
> > > early. */
> > > + if (!ctxt)
> > > + {
> > > + cd.set_any_lhs_with_defaults ();
> > > + return;
> > > + }
> >
> > Hmmm, I don't like the way the patch has
> > cd.set_any_lhs_with_defaults ();
> > above and then repeats it below at the end of the function (DRY
> > principle). How about introducing a subroutine within the class to
> > do
> > the checks, so it looks like:
> >
> > if (ctxt)
> > if (!check_argument (cd))
> > return
> > cd.set_any_lhs_with_defaults ();
> >
> > or somesuch? Or does that make things messier?
>
> While thinking about this, I realized that calling
> `ctxt>terminate_path
> ()` when the argument to `mkstemp` isn't a null-terminated string may
> be
> too aggressive. My original code followed the pattern used by
> `kf_strcat`, but `mkstemp` is more similar to `fopen`. `kf_fopen`
> sets
> the LHS to defaults regardless whether
> `check_for_null_terminated_string_arg` returns NULL. I have updated
> the
> code to do the same. Does this seem correct?
Looks good to me.
>
> If correct, it incidentally means now it's not necessary to return
> early
> from `impl_call_pre`, as I have extracted the template string check
> into
> a method, as suggested.
(nods)
>
> > > diff --git a/gcc/testsuite/gcc.dg/analyzer/mkstemp-1.c
> > > b/gcc/testsuite/gcc.dg/analyzer/mkstemp-1.c
> > > new file mode 100644
> > > index 00000000000..d8e0e51c39f
> > > --- /dev/null
> > > +++ b/gcc/testsuite/gcc.dg/analyzer/mkstemp-1.c
> > > @@ -0,0 +1,96 @@
> > > +/* { dg-additional-options "-Wno-analyzer-null-argument" } */
> > > +/* { dg-skip-if "has no putenv" { "avr-*-*" } } */
> >
> > The above skip-if line looks like it was a copy&paste from a test
> > of
> > "putenv"; it means that the test should be skipped on targets
> > matching
> > avr-*-* since they don't have "putenv".
> >
> > There might be targets that don't have mkstemp, but until we
> > discover
> > them, best to delete that line for this test.
>
> Ah yes, sorry, as you can see I based the `mkstemp` tests on the
> `putenv` tests and wasn't careful enough when updating it.
>
> > Other than those nitpicks, the patch looks great; thanks
Thanks for the updated patch. It looks good, and I've added it to my
queue of patches to push to gcc 17 once we reopen trunk for new
features.
Do you have a sense of what you'd like to look at next? You mentioned
a possible followup to cover similar functions to mkstemp. If you're
planning to apply to GSoC, I'm wondering what subproject you had in
mind. Note that adding a bunch of known functions to more fully handle
various POSIX APIs in -fanalyzer sounds like a reasonable GSoC project
idea to me; it would be useful and (I hope) relatively easy.
>
> Thank you. I have another question: I have noticed that generally any
> non-trivial method is defined out-of-line throughout the codebase.
> Would it be better to define `kf_mkstemp`'s private methods out-of-
> line
> as well?
I don't have a strong preference here
Dave
