Page MenuHomeFreeBSD

linux: For better compatibility, provide compatible endian.h
ClosedPublic

Authored by imp on Sep 14 2021, 7:40 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Dec 9, 4:50 AM
Unknown Object (File)
Sun, Dec 8, 9:56 PM
Unknown Object (File)
Sat, Dec 7, 10:50 AM
Unknown Object (File)
Fri, Dec 6, 3:56 AM
Unknown Object (File)
Fri, Dec 6, 12:03 AM
Unknown Object (File)
Fri, Dec 6, 12:03 AM
Unknown Object (File)
Fri, Dec 6, 12:00 AM
Unknown Object (File)
Fri, Dec 6, 12:00 AM

Details

Summary

Add endian.h. This includes sys/endian.h and then adds extra defines
that glibc defines with double underscores for our
_{BIG,BYTE,LITTLE,PDP}_ENDIAN macros. We also define __FLOAT_WORD_ORDER
to be the same as _BYTE_ENDIAN since FreeBSD doesn't currently define
this, and the default with glibc is exactly this for our platforms.
Move common parts of endian.h and sys/endian.h into sys/_endian.h
to limit namespace pollution from endian.h

All this gives us good compatibility with Linux.

There are some minor differences:

o The extra glibc macros are not defined. These are all
    controlled with either __ at the start, or only defined
    when glibc is being built. We also don't define macros
    that are used internally in glibc that would pollute
    the namespace.
o For complete compatibility, this change must also be
    paired with providing a glibc-compatible byteswap.h.
Test Plan

I think this is likely an exp-run level change.
I have a recollection of doing this before, but can't find any reviews enshrining that (it may have been just for nvme-cli)
Not sure who else to add to the review.
The two different APIs are close, but not identical and I tried to chase down all the differences and implement or document them.

https://reviews.freebsd.org/D19500 tried to do this before, but it was never committed. I've included the feedback / info from it.

I've done an exp run, written some tests to try to codify what I learned and I've reworked it. Will do another exp run
after people have a chance to reply to the last set of changes. I've updated the commit message above too.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 41635
Build 38524: arc lint + arc unit

Event Timeline

imp requested review of this revision.Sep 14 2021, 7:40 PM
imp created this revision.
imp added a reviewer: jhb.
imp added reviewers: markj, zeising.
imp edited the test plan for this revision. (Show Details)

bring in prior attempt

include/endian.h
9
16
18

update with markj's typo / grammar corrections
Clarify a few more details.

imp marked 3 inline comments as done.Sep 15 2021, 2:34 PM
This revision is now accepted and ready to land.Sep 15 2021, 3:12 PM
include/endian.h
30

Is it not possible to achieve this another way? That is, if something includes both sys/endian.h and then endian.h it ends up without this which I think is not the expected behavior? That is, should this be handled by a __OMIT_BSWAP or the like?

include/endian.h
30

If you include both, that's undefined. One or the other is the API.
We don't know if sys/endian.h got included via namespace pollution before this point in the compilation unit. If it did, then there's not much that we can do (though there's nothing I see in base that would likely be included by a linux program).
If sys/endian.h is included after this, nothing will happen because the multiple include guard will prevent it from having any effect at all.
I'm not sure that having this #define will make things better given the order-dependent weirdness that's already present.
Though if we did do something like that, I'd prefer to call it something else. Maybe __ENDIAN_H_STRICT for the name....
The other option is to decompose sys/endian.h further, but I'm not sure the value in that.

Please do an exp-run before this goes in. At least mesa ports provide their own endian stuff in some cases and should be checked that they still are OK.

Please do an exp-run before this goes in. At least mesa ports provide their own endian stuff in some cases and should be checked that they still are OK.

Exp run requested. Waiting on the results.

The quick analysis of the failures suggests
(a) We also need a byteorder.h
(b) People are bogusly setting _POSIX_C_SOURCE to some value, and then testing to see if non-standard things are defined. Doh!

Due to (a) I plan on creating a byteorder.h that's going to be somewhat similar to the byteorder.h we have for infiniband (but it can't be the same) as well as dropping the __BSD_VISIBLE guards because (b).

regen after sys/_endian.h update

This revision now requires review to proceed.Sep 21 2021, 7:23 PM

@jhb I take back my comments about the order being undefined.
mail/vpopper does weird stuff and absolutely needs to be supported for sys/endian.h and then endian.h
whilst databases/galara needs the opposite.
Both, when built on FreeBSD, need bswap16 defined because of the weird way that they detect if bswap16 is defined and use it instead of __bswap16
So it has to be well defined. To do that I've moved the common to sys/_endian.h and eliminated the ugly OMIT hack.

imp marked an inline comment as done.Sep 21 2021, 8:07 PM
imp added inline comments.
include/endian.h
30

I've reworked a little so that things are more compatible. You can now include both and get the union.

imp added a reviewer: mhorne.
imp edited the summary of this revision. (Show Details)
imp edited the test plan for this revision. (Show Details)

Seems straightforward. Are there any manpages that need updating? Should FreeBSD sources still prefer sys/endian.h going forward, and is that mentioned anywhere?

Edit: what I mean to ask is: is the use of #include <endian.h> discouraged in the src tree?

include/Makefile
12

Possibly time for a one-item-per-line cleanup here?

sys/sys/endian.h
60
This revision is now accepted and ready to land.Sep 22 2021, 5:01 PM
include/Makefile
12

It's an insta-MFC conflict engine (though it will help future branches) so I want to separate that out.
It's also a conflict generator for -current. I gotta wait for one maybe two more exp runs on this stuff and want to
keep any possible conflicts to a minimum in that time.
So I may do this, but as a follow-on commit after I've MFC'd this. It is a good suggestion in general.

sys/sys/_endian.h
35

I wonder if we really need this, but I retained it.

sys/sys/endian.h
60

thanks.

jhb added inline comments.
sys/sys/endian.h
63
imp marked 2 inline comments as done.Sep 23 2021, 7:43 PM

Made the minor tweaks suggested so far... will generate a new exprun request COB today.

imp edited the summary of this revision. (Show Details)

rebase / update / tweak per review

This revision now requires review to proceed.Sep 23 2021, 10:27 PM
This revision is now accepted and ready to land.Sep 24 2021, 1:12 PM

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=258544 contains the exp run information.

There's 4 ports that I need to fix according the last exp run report:

  • util-linux (pull request upstream, email to yuri@ the maintainer to bring it in)
  • rmilter (need to fix)
  • mosh (need to fix)
  • segyio (need to fix but should be similar to util-linux)