Page MenuHomeFreeBSD

Support clang-specific type safety attributes in fcntl(2)
AbandonedPublic

Authored by brooks on Apr 30 2015, 5:01 PM.

Details

Reviewers
theraven
pfg
Group Reviewers
manpages
Summary

Clang has some new type safety attributes for fcntl() or ioctl(), which
lets the compiler know that an argument depends on another and also
tell the compiler which values are valid for such functions.

http://clang.llvm.org/docs/AttributeReference.html#type-safety-checking

The extra support requires that we annotate the (fcntl) declaration and
tag each of the valid values. Since attributes are associated with
variables, and you cannot set these on a #define, this requires changing
the fcntl() constants into static consts.

As an end result the compile will warn about suspicious values when
calling fcntl.

It would be really nice to do something similar with ioctls but that
involves changing the way we generate the tables and I don't have good
ideas on how to handle that.

Test Plan

Tinderbox.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

pfg retitled this revision from to Support clang-specific type safety attributes in fcntl(2).Apr 30 2015, 5:01 PM
pfg updated this object.
pfg edited the test plan for this revision. (Show Details)
pfg added reviewers: theraven, brooks, dim, emaste.
pfg updated this revision to Diff 5116.
pfg abandoned this revision.May 5 2015, 9:49 PM

This is/was wrong: the fcntl attribute has to include the type for the next argument and in the case of F_GETLK it is not an integer:

> lib/libc (obj,depend,all,install)

/scratch/tmp/pfg/head/lib/libc/gen/lockf.c:65:32: error: argument type 'struct f
lock *' doesn't match specified '_fcntl' type tag that requires 'int' [-Werror,-Wtype-safety]

if (_fcntl(filedes, F_GETLK, &fl) == -1)
                    ~~~~~~~  ^~~

1 error generated.
...


Unfortunately the fcntl() variations are not well documented to build the annotations effectively..

This was only meant as a proof-of-concept exercise anyways.

pfg removed rS FreeBSD src repository as the repository for this revision.
pfg updated this revision to Diff 5388.

This is a proof of context, it goes beyond the previous attempts and effective defines the attributes. Unfortunately defining the fcntls as static consts breaks some legacy code that would have to be rewritten.

pfg abandoned this revision.May 14 2015, 3:40 PM

I'll just give up as this requires rewriting some legacy code. I will also be dropping the attributes from cdefs.h.

theraven edited edge metadata.May 14 2015, 4:41 PM

What things needed rewriting to work with this?

pfg added a comment.May 14 2015, 6:33 PM

What things needed rewriting to work with this?

contrib/openbsm/libbsm/bsm_fcntl.c (around line 48)

Makes use of fcntl values as initializers and breaks the build.
There are probably other cases but given the difficulty for
doing something similar with ioctls I am wondering if it makes
sense at all.

I think I'll leave the definitions in cdefs.h, there are probably
new code cases where this functionality may find uses.

Hmm, that doesn't look as if it should break. The macros still expand to an integer constant expression. You'll lose the type checking when you do this, but the initialisation should still work. What is the compiler error that you see?

pfg added a comment.May 15 2015, 4:33 PM

Hmm, that doesn't look as if it should break. The macros still expand to an integer constant expression. You'll lose the type checking when you do this, but the initialisation should still work. What is the compiler error that you see?

Sorry I erased the build log, it was something about the value not being a constant.

I think we would have to have a compatibility mode for this type of situations. It doesn't look difficult to add one in the fcntl case but it may be ugly.

brooks added a reviewer: pfg.May 9 2016, 6:07 PM
brooks commandeered this revision.
brooks added a subscriber: brooks.

I need some sort of annotations like this for CheriBSD so I'm grabbing this as a starting point.

pfg edited edge metadata.May 9 2016, 7:06 PM

Just IMHO ... we could probably do without any compatibility #defines and just leave static const ints.
I think this will cause (fixable) issues only in the base system.

It will be a mess for Ioctls, but it is doable as well.