Page MenuHomeFreeBSD

Add endian.h to LHDRS.
Needs ReviewPublic

Authored by markj on Mar 7 2019, 8:44 PM.

Details

Summary

Many applications expect to be able to include <endian.h> so this is a
compatibility sore. Just make a symlink, like OpenBSD does. NetBSD
uses a stub header which includes sys/endian.h, but I don't see a good
reason for that. We use symlinks for lots of headers today.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 23086
Build 22146: arc lint + arc unit

Event Timeline

markj created this revision.Mar 7 2019, 8:44 PM
emaste added a comment.Mar 7 2019, 8:50 PM

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

brooks accepted this revision.Mar 7 2019, 8:50 PM

LGTM.

This revision is now accepted and ready to land.Mar 7 2019, 8:50 PM
emaste accepted this revision.Mar 7 2019, 8:51 PM
markj added a comment.Mar 7 2019, 8:54 PM

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?

emaste added a comment.Mar 7 2019, 9:21 PM

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.

zeising added a subscriber: zeising.Mar 9 2019, 6:04 PM

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

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

zeising added a comment.EditedMar 11 2019, 1:11 PM

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.

zeising requested changes to this revision.Mar 11 2019, 1:35 PM
This revision now requires changes to proceed.Mar 11 2019, 1:35 PM
kevans added a subscriber: kevans.Mar 11 2019, 2:01 PM
markj added a reviewer: imp.Mar 11 2019, 5:05 PM
markj updated this revision to Diff 54936.Mar 11 2019, 5:05 PM

Introduce a wrapper and define __BYTE_ORDER etc.

markj added a comment.Mar 11 2019, 5:09 PM

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.

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...

markj added a comment.Mar 11 2019, 6:05 PM

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?

imp added a comment.EditedMar 12 2019, 5:55 AM

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 */

markj updated this revision to Diff 55059.Mar 14 2019, 1:44 PM
  • Don't define bswap*() in <endian.h>.
markj added a comment.Mar 14 2019, 1:46 PM
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.

imp added a comment.Mar 14 2019, 11:24 PM
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.

markj added a comment.Mar 19 2019, 2:54 AM
In D19500#419336, @imp 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.

markj added a comment.Mar 26 2019, 7:14 PM

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

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?

kevans added a comment.Jan 4 2020, 4:03 PM

Ping? Ran face-first into this again today.

markj added a comment.Jan 5 2020, 6:22 PM

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.