Fine with me; should we have an exp-run?

In D19500#417356, @emaste wrote:

Fine with me; should we have an exp-run?

Well, if anything breaks with this change we'll want to fix the ports rather than revert the change. Is an exp-run called for in that kind of scenario?

In D19500#417373, @markj wrote:

In D19500#417356, @emaste wrote:

Fine with me; should we have an exp-run?

Well, if anything breaks with this change we'll want to fix the ports rather than revert the change. Is an exp-run called for in that kind of scenario?

The intent is just to catch (and fix) those kinds of issues before commit, in case it e.g. breaks some common dependency.

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=236398

Hi!
I would like to see this get an exp-run. There has been reports of issues in mesa with stray include/endian.h (which shouldn't happen), so it might be good to make a run just in case.
Thanks
Niclas

In D19500#417600, @markj wrote:

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=236398

Looks like most failures are caused by us not defining __BYTE_ORDER (we have _BYTE_ORDER though).

Hi!
This change causes issues with mesa-dri.
Previously, mesa included machine/endian.h, which is different from sys/endian.h (for reasons I don't understand). Now it detects include/endian.h and tries to use that. However, include/endian.h defines bswap32() as a macro, and machine/endian.h doesn't. mesa has it's own bswap32() function, which, as far as I can see, is always used (it calles a __builtin if possible).
Looking at the two linuxs I have in front of me, I can't see bswap32() defined in include/endian.h in either, and compiling a program that simply includes endian.h and calls bswap32 gives warnings about implicit function declaration bswap32.

We probably also need to define

__BYTE_ORDER, __LITTLE_ENDIAN and __BIG_ENDIAN

, at least looking at the mesa source, not having those will probably also cause issues.

If we are doing this, we probably need our endian.h to be more similar.

I might be able to teach mesa to ignore endian.h and use machine/endian.h as is done now, or to cope with our endian.h, but since this is done to be more compatible with other projects, that feels like a step in the wrong direction.

So, I don't yet have a good solution for the mesa build. Hiding the bswap* declarations behind an #if __BSD_VISIBLE guard didn't fix the problem there.

I'm inclined to think that because mesa is already including machine/endian.h, it "knows what it's doing" and should perhaps continue to do that instead of including endian.h.

In D19500#418222, @markj wrote:

So, I don't yet have a good solution for the mesa build. Hiding the bswap* declarations behind an #if __BSD_VISIBLE guard didn't fix the problem there.

I'm inclined to think that because mesa is already including machine/endian.h, it "knows what it's doing" and should perhaps continue to do that instead of including endian.h.

I can probably make it do that, or use the system bswap32. However, it would not surprise me that this crops up elsewhere, since Linux does not define bswap*().
I believe the reason mesa defines it's own bswap32() is because Linux doesn't, and so far, we haven't either, at least not in machine/endian.h.

It just feels to me that we're doing this to be compatible, and then we need to be compatible...

In D19500#418232, @zeising wrote:

In D19500#418222, @markj wrote:

So, I don't yet have a good solution for the mesa build. Hiding the bswap* declarations behind an #if __BSD_VISIBLE guard didn't fix the problem there.

I'm inclined to think that because mesa is already including machine/endian.h, it "knows what it's doing" and should perhaps continue to do that instead of including endian.h.

I can probably make it do that, or use the system bswap32. However, it would not surprise me that this crops up elsewhere, since Linux does not define bswap*().

Linux does define an equivalent macro, bswap_32() in byteswap.h.

FWIW, none of the other exp-run failures were caused by bswap*.

I believe the reason mesa defines it's own bswap32() is because Linux doesn't, and so far, we haven't either, at least not in machine/endian.h.

It just feels to me that we're doing this to be compatible, and then we need to be compatible...

The only other option I can see is to export bswap*() conditionally. __BSD_VISIBLE is insufficient. #ifdef _KERNEL is probably too strong a restriction (though there is not much in the base system that uses it).

One other option is to not export bswap*() if _ENDIAN_H_ is defined, i.e., only export the "non-standard" macros if sys/endian.h was included directly. This will most likely fix the problem with mesa, but is rather ugly. What do folks think?

Here's what I had come up with for a glibc compatable header. Feel free to steal any or all of this. Foundation copyright is fine.

I didn't bother to fix the namespace pollution when I did this, but I was never targeting MESA. I did this to save me from hacking a dozen places in nvme-cli. The pollution was fine for the nvme-cli.

/* Copyright here */

/*

• Linux / glibc compatibility iterface for <endian.h> which differs from
• the BSD standard in a number of trivial and annoying ways, plus provides
• a few extras not found in BSD. *
• The implementation also name-pollutes bswap* and *_identity, which we
• do not. We have our own namespace pollution instead. Should those be required
• they can be provided. */

#ifndef ENDIAN_H
#define ENDIAN_H 1

/* FreeBSD defines most things in sys/endian.h */

#include <sys/endian.h>

/* Linux / glibc compatible interfaces */

#define BYTE_ORDER _BYTE_ORDER
#define LITTLE_ENDIAN _LITTLE_ENDIAN
#define BIG_ENDIAN _BIG_ENDIAN
#define PDP_ENDIAN _PDP_ENDIAN

/* Linux / glibc expects a float order too, FreeBSD doesn't have one, so assume smame */

#define __FLOAT_WORD_ORDER _BYTE_ORDER

/* Linux / glibc also allows people to use non-underscore prefixed versions */

#ifdef __USE_MISC
#define LITTLE_ENDIAN _LITTLE_ENDIAN
#define BIG_ENDIAN _BIG_ENDIAN
#define PDP_ENDIAN _PDP_ENDIAN
#define BYTE_ORDER _BYTE_ORDER
#endif

/* long long pair interface */
#if _BYTE_ORDER == _LITTLE_ENDIAN
#define LONG_LONG_PAIR(HI, LO) LO, HI
#elif _BYTE_ORDER == _BIG_ENDIAN
#define LONG_LONG_PAIR(HI, LO) HI, LO
#endif

#endif /* ENDIAN_H */

In D19500#418483, @imp wrote:

Here's what I had come up with for a glibc compatable header. Feel free to steal any or all of this. Foundation copyright is fine.

I didn't bother to fix the namespace pollution when I did this, but I was never targeting MESA. I did this to save me from hacking a dozen places in nvme-cli. The pollution was fine for the nvme-cli.

For now I'm just trying to create a minimal header that survives an exp-run; we can always add to it.

For mesa, I explicitly #undef the bswap macros. I considered adding a sys/bswap.h instead, but it breaks a few userspace things and there's some precedent for undef'ing names in our standard headers.

In D19500#419197, @markj wrote:

In D19500#418483, @imp wrote:

Here's what I had come up with for a glibc compatable header. Feel free to steal any or all of this. Foundation copyright is fine.

I didn't bother to fix the namespace pollution when I did this, but I was never targeting MESA. I did this to save me from hacking a dozen places in nvme-cli. The pollution was fine for the nvme-cli.

For now I'm just trying to create a minimal header that survives an exp-run; we can always add to it.

OK. If we're doing an exp run, then maybe we should include my stuff so we can do just one. It was done by examining a number of different versions of endian.h to figure out what Linux users want to the largest extent possible w/o violating POLA...

For mesa, I explicitly #undef the bswap macros. I considered adding a sys/bswap.h instead, but it breaks a few userspace things and there's some precedent for undef'ing names in our standard headers.

I think these changes are good.

In D19500#419336, @imp wrote:

In D19500#419197, @markj wrote:

In D19500#418483, @imp wrote:

Here's what I had come up with for a glibc compatable header. Feel free to steal any or all of this. Foundation copyright is fine.

I didn't bother to fix the namespace pollution when I did this, but I was never targeting MESA. I did this to save me from hacking a dozen places in nvme-cli. The pollution was fine for the nvme-cli.

For now I'm just trying to create a minimal header that survives an exp-run; we can always add to it.

OK. If we're doing an exp run, then maybe we should include my stuff so we can do just one. It was done by examining a number of different versions of endian.h to figure out what Linux users want to the largest extent possible w/o violating POLA...

I had already asked for one with my minimal change. As it turns out, undef'ing bswap*() is fragile: there are ports that #include endian.h followed by sys/endian.h and want to use bswap*() macros.

If we're going to fix this at all I think the only sane way to go is to add a sys/bswap.h, like NetBSD. When _KERNEL is defined, we can have sys/endian.h include sys/bswap.h so that we don't have to fix up all the kernel consumers. There are some userland consumers (OFED and libefi off the top of my head) which will need to be fixed. I'll create a patch, roll up your extra Linux compat defines into endian.h, fix in-tree userland consumers, and request another exp-run.

In D19500#420345, @markj wrote:

If we're going to fix this at all I think the only sane way to go is to add a sys/bswap.h, like NetBSD. When _KERNEL is defined, we can have sys/endian.h include sys/bswap.h so that we don't have to fix up all the kernel consumers. There are some userland consumers (OFED and libefi off the top of my head) which will need to be fixed. I'll create a patch, roll up your extra Linux compat defines into endian.h, fix in-tree userland consumers, and request another exp-run.

I think this is probably too painful. There's a ton of things in the base system (including contrib software) that depends on the bswap* pollution from sys/endian.h: crypto headers, opie, libefi, OFED, SMB, makefs. And, I noticed that NetBSD's endian.h creates the same pollution even though they have a separate machine/bswap.h.

@zeising so far I think mesa-dri is the only port that gets fallout from having a /usr/include/endian.h that defines bswap* macros. If it turns out that no other ports require patching, would you consider adding a patch handle this in the one file that requires it? It is not a very satisfying solution, but it seems by far the least painful. I note that NetBSD's pkgsrc has a patch for the same issue: https://github.com/NetBSD/pkgsrc/blob/trunk/graphics/MesaLib18/patches/patch-src_mesa_drivers_dri_i965_intel__tiled__memcpy.c

In D19500#422343, @markj wrote:

In D19500#420345, @markj wrote:

If we're going to fix this at all I think the only sane way to go is to add a sys/bswap.h, like NetBSD. When _KERNEL is defined, we can have sys/endian.h include sys/bswap.h so that we don't have to fix up all the kernel consumers. There are some userland consumers (OFED and libefi off the top of my head) which will need to be fixed. I'll create a patch, roll up your extra Linux compat defines into endian.h, fix in-tree userland consumers, and request another exp-run.

I think this is probably too painful. There's a ton of things in the base system (including contrib software) that depends on the bswap* pollution from sys/endian.h: crypto headers, opie, libefi, OFED, SMB, makefs. And, I noticed that NetBSD's endian.h creates the same pollution even though they have a separate machine/bswap.h.

@zeising so far I think mesa-dri is the only port that gets fallout from having a /usr/include/endian.h that defines bswap* macros. If it turns out that no other ports require patching, would you consider adding a patch handle this in the one file that requires it? It is not a very satisfying solution, but it seems by far the least painful. I note that NetBSD's pkgsrc has a patch for the same issue: https://github.com/NetBSD/pkgsrc/blob/trunk/graphics/MesaLib18/patches/patch-src_mesa_drivers_dri_i965_intel__tiled__memcpy.c

Ping?

Ping? Ran face-first into this again today.

In D19500#504759, @kevans wrote:

Ping? Ran face-first into this again today.

I'll revisit this this week and try another exp-run to see if the damage is still limited to mesa.

I'll start pushing this into the tree https://reviews.freebsd.org/D31962 has the review.

https://reviews.freebsd.org/D31964 has the easier part of this too...
Just to document things.