sys/conf: enable -fms-extensions for the kernel

[bz]

Summary

In preparation to de-couple some of LinuxKPI-USB from native USB
enable MS extensions allowing us to embed anonymous structs or unions
in other structs/unions. While this is a non-standard C extension
both compilers have supported it long enough to be able to use it and
it gives a lot more readable code than using macros for struct fields,
especially with lots of pre-processor conditions.

The reason to enable it globally for the kernel is that otherwise the
options (thorugh a define) would have to be sprinkled through half
of the USB stack and drivers, which, even just for amd64, lead to a
very messy outcome, hard to maintain.

Sponsored by: The FreeBSD Foundation
Discussed with: srcmgr (imp)
MFC after: 3 days

[bz]

I am leaving the review set to srcmgr first based on the emails I sent; we may add more people as we see fit if srcmgr in general agrees.

[imp]

I like it, but it it was my idea... I'll let someone else on Srcmgr approve to give others a chance

[jhb]

Hmmm, I didn't think we required the CFLAGS for this? We already use anonymous structs and unions all over the kernel.

Oh, we already have this for the kernel for GCC at least:

sys/conf/kern.pre.mk:CFLAGS.gcc+= -fms-extensions -finline-limit=${INLINE_LIMIT}
sys/conf/kern.pre.mk:CFLAGS.gcc+= -fms-extensions
sys/conf/kmod.mk:CFLAGS.gcc+=   -fms-extensions

In fact, we have it twice for no good reason in kern.pre.mk. I'm surprised we need any clang changes given the widespread use of this already in the kernel. There could be some general value in moving duplicate settings in kern.pre.mk and kmod.mk into kern.mk, but that is a different change.

[bz]

In D54986#1258338, @jhb wrote:

Hmmm, I didn't think we required the CFLAGS for this? We already use anonymous structs and unions all over the kernel.

Oh, we already have this for the kernel for GCC at least:

sys/conf/kern.pre.mk:CFLAGS.gcc+= -fms-extensions -finline-limit=${INLINE_LIMIT}
sys/conf/kern.pre.mk:CFLAGS.gcc+= -fms-extensions
sys/conf/kmod.mk:CFLAGS.gcc+=   -fms-extensions

In fact, we have it twice for no good reason in kern.pre.mk. I'm surprised we need any clang changes given the widespread use of this already in the kernel. There could be some general value in moving duplicate settings in kern.pre.mk and kmod.mk into kern.mk, but that is a different change.

Yes, enabling this generally and cleaning up the modules Makefiles should indeed be a follow-up or part of this. Thanks for pointing this out.

BUT no, it's not what you think!

See the USB review next in the stack (or the email I just replied to.

I would assume the standard allows you to do:

struct foo {
        struct {
                int x;
        };
        int y;
};

which we indeed use everywhere. MS-extensions allow you to do:

struct _bar {
        int x;
};

struct foo {
        struct _bar;
        int y;
};

struct foo.x = 17;

[glebius]

struct _bar {
        int x;
};

struct foo {
        struct _bar;
        int y;
};

struct foo.x = 17;

That's pretty evil! I don't think we should encourage that around the kernel. C11 anonymous unions & structs don't leave any space for ambiguity, they do quite the opposite. This extension looks pretty scary, as I may have x in bar and in baz, and then make both baz and bar member of foo.

[imp]

In D54986#1258387, @glebius wrote:

struct _bar {
        int x;
};

struct foo {
        struct _bar;
        int y;
};

struct foo.x = 17;

That's pretty evil! I don't think we should encourage that around the kernel. C11 anonymous unions & structs don't leave any space for ambiguity, they do quite the opposite. This extension looks pretty scary, as I may have x in bar and in baz, and then make both baz and bar member of foo.

We're rather stuck with this, though, because that's what the Linux USB stack uses for its interface. And to get LinuxKPI for USB (which we need for wireless), bz wants to move some of into the core USB structures. And to do that, we need to add the ms-extentions to a lot of USB drivers since it's part of the core USB stack now. bz can go into the very good reasons why we have to do it that way.

So I totally agree we should encourage newer C11 forms, absolutely, but there's a bigger picture set of reasons here for this.

[bz]

In D54986#1258387, @glebius wrote:

struct _bar {
        int x;
};

struct foo {
        struct _bar;
        int y;
};

struct foo.x = 17;

That's pretty evil! I don't think we should encourage that around the kernel. C11 anonymous unions & structs don't leave any space for ambiguity, they do quite the opposite. This extension looks pretty scary, as I may have x in bar and in baz, and then make both baz and bar member of foo.

that just fails with error: duplicate member 'x'

But it's funny given jhb in an email thread pointed out that it was you moving this globally enabled in the kernel for gcc in cc4a90c445aa04be36c3ef745cbe67fa339b94b5 . I don't think that commit did what you thought it would back then.

[glebius]

In D54986#1258395, @bz wrote:

In D54986#1258387, @glebius wrote:

struct _bar {
        int x;
};

struct foo {
        struct _bar;
        int y;
};

struct foo.x = 17;

That's pretty evil! I don't think we should encourage that around the kernel. C11 anonymous unions & structs don't leave any space for ambiguity, they do quite the opposite. This extension looks pretty scary, as I may have x in bar and in baz, and then make both baz and bar member of foo.

that just fails with error: duplicate member 'x'

But it's funny given jhb in an email thread pointed out that it was you moving this globally enabled in the kernel for gcc in cc4a90c445aa04be36c3ef745cbe67fa339b94b5 . I don't think that commit did what you thought it would back then.

That commit added what it intended. The gcc of that time was not C11-compliant, and the "Microsoft extensions" was the only way to enable part of C11 that we actively use.

Back to today's problem. I'd suggest to add -ms-extensions to clang build only for LinuxKPI for USB files and don't add it globally.

[bz]

We're rather stuck with this, though, because that's what the Linux USB stack uses for its interface. And to get LinuxKPI for USB (which we need for wireless), bz wants to move some of into the core USB structures. And to do that, we need to add the ms-extentions to a lot of USB drivers since it's part of the core USB stack now. bz can go into the very good reasons why we have to do it that way.

Okay I'll try again.

Our native USB stack has lots of #ifdef around structures and includes bits on-demand.

Linux uses the same structures and field names for a lot of things and the (old) LinuxKPI USB implementation is a wrapper around our native USB stack.

One of the many things which are included in the native USB stack include are the Linux[KPI]-specific field members.

You can see this here:
https://cgit.freebsd.org/src/tree/sys/dev/usb/usb_device.h#n274

None of this is based on my brain.

Our LinuxKPI USB implementation is 10-15 years old and IMHO not used by anything anymore.

What I am trying to do is update it so that modern Linux USB drivers can use our LinuxKPI USB implementation again. In my case for wireless drivers.

I am now facing that I need to embed the Linux "struct device dev' (not as a pointer) into two of these structs.
trying to pull in the Linux[KPI] struct device into our native USB implementation is a nightmare and really a no-go; also given that struct keeps changing and changes size etc.

My first iteration like 2 years ago I started to take the native USB structs and made them a #define USB_DEVICE and dealt with all the #ifdef that this struct contains. The remaining structure definition is unreadable. I then replaced the LinuxKPI fields in that native struct with an array to hold the Linux[KPI] fields.

Then I did the same as I done now: I have a struct usb_device in the native stack and a struct usb_device in LinuxKPI. Both first include all the common native fields. The the native simply has the array and the LinuxKPI one has the LinuxKPI struct fields.

So all I did was duplicate the struct(s) removing the LinuxKPI fields from the native implementation and only having them in the LinuxKPI version. That solved more than 50% of all the overlap troubles we have and allowed me to update the LinuxKPI USB implementation and compile modern Linux USB drivers against it, while our native stack still drives most of the work and logic and LinuxKPI is once again a translation layer as needed.

What the -fms-extensions solve is simply avoiding ugly unmaintainable and unreadable structure definitions as a #define.

The reason (after a discussion) this went globally is that our USB bits are splattered and both sys/conf/files and various modules would have needed the addition of -fms-extension to limit the scope where this feature would be usable. With that imp suggested to just make it globally in the kernel. As jhb then pointed out we've had this on globally for more than a decade for gcc but in fact no-one actually used the feature as it was meant (it seems).

So all-in-all all I did was really remove the Linux[KPI] struct fields from the native USB implementation where they were embedded for ages in order to be able to add other fields which would conflict with any native implementation or make it impossible to get anything compiling without pulling half of LinuxKPI into our native USB stack.

I hope this clarifies some of the reason why I am asking for this.

[bz]

And to give you an idea how struct usb_device looks like if you do the #define path avoiding -fms-extensions (including all the #ifdef, which if someone thinks we should remove and solve be my guest).
If anyone has alternative suggestion on how to hide the LinuxKPI fields form the native implementation but keeping the struct size the same I'll happily listen.

#if (USB_HAVE_FIXED_IFACE == 0)
#define USB_DEVICE_USB_INTERFACES                                       \
        struct usb_interface *ifaces;
#else
#define USB_DEVICE_USB_INTERFACES                                       \
        struct usb_interface ifaces[USB_IFACE_MAX];
#endif

#if (USB_HAVE_FIXED_ENDPOINT == 0)
#define USB_DEVICE_USB_ENDPOINT                                         \
        struct usb_endpoint *endpoints;
#else
#define USB_DEVICE_USB_ENDPOINT                                         \
        struct usb_endpoint endpoints[USB_MAX_EP_UNITS];
#endif

#if USB_HAVE_UGEN
#define USB_DEVICE_HAVE_UGEN                                            \
        struct usb_fifo *fifo[USB_FIFO_MAX];                            \
        struct usb_symlink *ugen_symlink;       /* our generic symlink */ \
        struct usb_fs_privdata *ctrl_dev;       /* Control Endpoint 0 device node */ \
        SLIST_HEAD(,usb_fs_privdata) pd_list;                           \
        char    ugen_name[20];          /* name of ugenX.X device */
#else
#define USB_DEVICE_HAVE_UGEN
#endif

#if (USB_HAVE_FIXED_CONFIG != 0)
#define USB_DEVICE_CONFIG_DATA                                          \
        uint32_t config_data[(USB_CONFIG_MAX + 3) / 4];
#else
#define USB_DEVICE_CONFIG_DATA
#endif

#define USB_DEVICE_DEF                                                  \
        /* statistics */                                                \
        struct usb_device_statistics stats_err;                         \
        struct usb_device_statistics stats_ok;                          \
        struct usb_device_statistics stats_cancelled;                   \
                                                                        \
        /* generic clear stall message */                               \
        struct usb_udev_msg cs_msg[2];                                  \
        struct sx enum_sx;                                              \
        struct sx sr_sx;                                                \
        struct sx ctrl_sx;                                              \
        struct mtx device_mtx;                                          \
        struct cv ctrlreq_cv;                                           \
        struct cv ref_cv;                                               \
                                                                        \
        USB_DEVICE_USB_INTERFACES                                       \
        struct usb_endpoint ctrl_ep;    /* Control Endpoint 0 */        \
        USB_DEVICE_USB_ENDPOINT                                         \
        struct usb_power_save pwr_save;/* power save data */            \
        struct usb_bus *bus;            /* our USB BUS */               \
        device_t parent_dev;            /* parent device */             \
        struct usb_device *parent_hub;                                  \
...
        struct usb_endpoint *ep_curr;   /* current clear stall endpoint */ \
        USB_DEVICE_HAVE_UGEN                                            \
        usb_ticks_t plugtime;           /* copy of "ticks" */           \
...

/* Hide structure from LinuxKPI which has its own copy */
#ifndef LINUXKPI_USB
struct usb_device {
        USB_DEVICE_DEF
#if USB_HAVE_COMPAT_LINUX
        /*
         * Extend structure size to accomodate for LinuxKPI extensions.  
         * linuxkpi/usb has a CTASSERT making sure USB_DEVIE_DEF +
         * LINUXKPI_USB_DEVICE_DEF_EXTENSION is at least as large as
         * its view of its private structure.
         */
        LINUXKPI_USB_DEVICE_DEF_EXTENSION
#endif
};
#endif

The alternative using -fms-extensions is here:
https://reviews.freebsd.org/differential/changeset/?ref=1489748

Asking to limit the scope of -fms-extensions (which was my initial intention before discussions started), this was amd64 only with a partial hammer in the end and limiting it to only the lines/modules needed, one already sees netgraph, sound, some native USB drivers outside sys/dev/usb/, hid but only two of the list of modules, ... and that list will grow over time. I am thankful I didn't have to try arm* anymore a week later.

 sys/conf/files                                   | 128
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-------------------------------------------
 sys/conf/kern.pre.mk                             |   4 ++++
 sys/conf/kmod.mk                                 |   3 +++
 sys/dev/usb/usb_device.c                         |   5 +----
 sys/dev/usb/usb_device.h                         |  29 +++++++++++++++++------------
 sys/dev/usb/usbdi.h                              |  25 +++++++++++++++++--------
 sys/modules/Makefile                             |   1 +
 sys/modules/hid/Makefile                         |   1 +
 sys/modules/hid/bcm5974/Makefile                 |   2 ++
 sys/modules/hid/xb360gp/Makefile                 |   2 ++
 sys/modules/linuxkpi/Makefile                    |   1 -
 sys/modules/linuxkpi_usb/Makefile                |  14 ++++++++++++++
 sys/modules/netgraph/bluetooth/ubt/Makefile      |   2 ++
 sys/modules/netgraph/bluetooth/ubtbcmfw/Makefile |   2 ++
 sys/modules/otus/Makefile                        |   2 ++
 sys/modules/rtw88/Makefile                       |   2 ++
 sys/modules/rtwn_usb/Makefile                    |   2 ++
 sys/modules/sound/driver/uaudio/Makefile         |   2 ++
 sys/modules/usb/Makefile.inc                     |   1 +

[glebius]

First, let's talk about -fms-anonymous-structs then instead of -fms-extensions, cause the latter enables much more. Second, please don't use the fact that I added this flag back in 2015 for gcc as an argument. Gcc is our second tier compiler and back then this change was the least intrusive hack to allow us to go forward with C11 feature and still be compilable by back-then version of gcc. Actually, I should look if this change can be reverted today. Finally, the list of files you provided doesn't look scary to me. It is 12 modules out of more than 500.

[bz]

In D54986#1258458, @glebius wrote:

First, let's talk about -fms-anonymous-structs then instead of -fms-extensions, cause the latter enables much more.

It was added January 30, 2026 (c391efe6fb673) to the llvm-project git. Three days ago. What to talk about? Come back in 3 years to have enough support for it?

[glebius]

Oh, sorry for that. Let it be then -fms-extensions until -fms-anonymous-structs hits our version of clang. But for the files that need it.

My main concern is that while C11 feature makes code more readable, the MS feature allows to create a situation where you would spend a lot of time finding where x actually lives:

struct foobar {
  struct bar; // declared in bar_var.h
  struct foo; // declared in foo_int.h
  struct baz; // declared in baz_decls.h
} y;

y.x = 10;

And then you would need spend half an hour to find x, unless you opened your project in MSVC. Some people develop FreeBSD without MSVC :)

[bz]

In D54986#1258463, @glebius wrote:

Oh, sorry for that. Let it be then -fms-extensions until -fms-anonymous-structs hits our version of clang.

Agreed.

But for the files that need it.

My main concern is that while C11 feature makes code more readable, the MS feature allows to create a situation where you would spend a lot of time finding where x actually lives:

struct foobar {
  struct bar; // declared in bar_var.h
  struct foo; // declared in foo_int.h
  struct baz; // declared in baz_decls.h
} y;

y.x = 10;

And then you would need spend half an hour to find x, unless you opened your project in MSVC. Some people develop FreeBSD without MSVC :)

I have no idea about MSVC so that's on you ;-)

I don't think people will use it to that excess, and if they do I believe they deserve to go on a treasure hunt. The foo/bar/baz and y.x examples lack a bit of real world reality as the real variable names are like linux_iface_start. I admit "ep0" is harder but doing a grep for ep0 and usb I did locate it in a few seconds without even thinking, likewise for altsetting.

If you look at D54987 there's a clear comment in LinuxKPI pointing backward to the native implementation.
I can also add a forward comment from native to LinuxKPI but if you debug native you'll never look for any LinuxKPI variable names (and if you are the LinuxKPI guard and the fact that the variable lays inside an array called __linunxkpi make it fairly clear where to go to.

If you were to ask me, I would hope that people will not start using this feature excessively though we may see more of it given Linux did also (after multiple discussions over the years) globally enable it not too long ago, so we may start facing more vendor drivers using it somewhere internally anyway... https://git.kernel.org/pub/scm/linux/kernel/git/kbuild/linux.git/commit/?id=c4781dc3d1cf0e017e1f290607ddc56cfe187afc

[glebius]

and if they do I believe they deserve to go on a treasure hunt

The problem is that the other people would need to go on a treasure hunt, not those who create the problem.

I would hope that people will not start using this feature excessively

And I suggest to edit Makefiles in such a manner, that people would not be able to start using this feature.

[bz]

In D54986#1258550, @glebius wrote:

and if they do I believe they deserve to go on a treasure hunt

The problem is that the other people would need to go on a treasure hunt, not those who create the problem.

I would hope that people will not start using this feature excessively

And I suggest to edit Makefiles in such a manner, that people would not be able to start using this feature.

Well they could have had for more than a decade and didn't.

I think the question is whether you want to go on the treasure hunt and get all the cases in sys/conf/files* and sys/modules and possibly ports (kmod) [something I had not thought about initially .. ] and third party USB code clean by adding compile-with lines and CFLAGS+= ${EXTRA_USB_C} lines and then maintain and explain that to people for the next 10 years as they hit cases where their new code doesn't compile because of this missing, or if we just keep it clean and simple.

I'll wait what others think about this.

If I have to bit the bullet (to use Linus' words on this) and get this make universe and exp-run clean, then I'll likely switch back to the #define method outlined before which is ugly but contains the changes to 3 files and avoids trouble with all 3rd party code at the cost of maintaining these structures becoming very unpleasant. So people should think about it being 3 options not just two.

In the end I am here to do wifi (doing USB only as a necessity) and I am trying to somehow clean code up so we can move forward or these wifi bits will sit somewhere for the next two years again to rot away rather than passing packets.