Re: [PATCH v2 0/3] fcntl fortification

[Florian Weimer <fweimer@redhat.com>]

* Sergey Bugaev:

> On Tue, May 30, 2023 at 11:09 AM Florian Weimer <fweimer@redhat.com> wrote:
>> > 2. There is a __fcntl_types_compatible () macro which is a thin wrapper
>> >    over __builtin_types_compatible_p () in plain C, and uses an
>> >    std::is_same_v-like check (using partial template specialization) in
>> >    C++. Importantly, it uses __typeof () even in C++ (not decltype ()),
>> >    because we don't want the extra references appended to our type. For
>> >    example, we want 'int', not 'const int &' or 'int &&'.
>>
>> I think you should avoid using __typeof, otherwise we need to add
>> another GCC check.  If you need to use decltype, you'll have to add a
>> __cplusplus version check.
>
> I don't think I understand your point about the GCC check, could you
> please expand?

Ahh, maybe that check is implied by doing this for fortification only?

>> > 5. Here's the fcntl () macro in all of its horrible glory:
>> >
>> >    #define fcntl(fd, cmd, ...)
>> >      (__VA_OPT__ (0 ?) __fcntl_2_inline (fd, cmd)
>>
>> I think we should avoid the new __fcntl_2 symbol because it an
>> unnecessary optimization.
>
> And again I don't think I understand your point :|
> Could you please expand here as well? What's the (unnecessary)
> optimization here?
>
> __fcntl_2 is required to do runtime checking of whether the runtime
> value of CMD indeed does not require a third argument.

Oh, I'm not sure if the run-time check is really that useful.

There's no vfcntl function, so I expect that we will have accurate type
information at the callsite in most cases, and the compile-time check
works.

>> It would very nice if we could generate the appropriate warning for C
>> (-Wincompatible-pointer-types).  This is what I tried to do, but it
>> might actually be impossible.
>>
>> Should we generate errors for C++?  It requires compatible pointer
>> types, after all.
>
> So the way I think about this, what this is doing is not making fcntl
> into a proper type-safe overloaded function, but adding some
> safeguards on top of the existing vararg definition that catch some
> mistakes.
>
> The diagnostics emitted are different, and this is fine. Another
> instance of this: you get the "error: call to '__fcntl_missing_arg'
> declared with attribute error" and not the error you would otherwise
> get from GCC upon forgetting a required function argument ("error: too
> few arguments to function 'foo'").
>
> I would prefer to keep type mismatch a warning in C++ too (because
> this is best-effort additional safeguards, not a real type system),
> but this would be easy to change if you want me to.

Hmm.  I certainly don't see this as a blocker, what you are proposing is
way better than what we have today.

Thanks,
Florian