libfdt: avoid hosted headers for freestanding (EFI) consumers

[bluca]

We want to use libfdt in systemd's EFI binaries (stub and loader) to handle device trees. As EFI binaries are PE, this requires doing static "freestanding" builds.

libfdt_env.h includes stdlib.h and string.h unconditionally. This imports hosted libc declarations, whereas the EFI runtime provides its own declarations, and causes declaration conflicts.

Use the compiler-defined STDC_HOSTED value to include these headers only for hosted consumers. Normal userspace builds retain their existing behavior, while freestanding EFI consumers need no bespoke builds and can just include the static library shipped by distros.

[dgibson]

The usual approach for building libfdt in non-standard environments is to entirely replace libfdt_env.h for the build. In essence the included libfdt_env.h is just an example version that works for most Unix-like userspaces. Other enviroments, like inside kernels or firmwares will usually need their own version of libfdt_env.h.

For example, the Linux kernel has its own version here.

[bluca]

We are not including libfdt_env.h though, it's pulled in by libfdt.h itself. Shouldn't that be removed, if that's the case?

Also I'm not sure how that would work in practice, as we'd need to maintain the copy in sync (just with these two glibc headers inclusion skipped), so it would get out of date every time the library is updated in the distros, with a chance of incompatible changes breaking it. I'd much rather just use the header as provided by the library package, and not have to worry about it.

Is there any specific issue with this commit that makes it unsuitable?

[dgibson]

We are not including libfdt_env.h though, it's pulled in by libfdt.h itself. Shouldn't that be removed, if that's the case?

It depends on the details of your build system, but typically you can change include order (e.g. -I options) in order to have your custom libfdt_env.h override the included version.

Also I'm not sure how that would work in practice, as we'd need to maintain the copy in sync (just with these two glibc headers inclusion skipped), so it would get out of date every time the library is updated in the distros, with a chance of incompatible changes breaking it. I'd much rather just use the header as provided by the library package, and not have to worry about it.

libfdt_env.h doesn't have to provide much - the userspace version has much more than is actually needed, simply because pulling in the system headers we need includes much more than we actually need. libfdt is specifically designed to be compilable in unusual and limited environments, and libfdt_env.h is the mechanism. libfdt has deliberately minimal dependencies on the external environment and all of them come via libfdt_env.h - we never include system headers except indirectly via libfdt_env.h.

The exact things it needs to provide should be documented somewhere, but alas we never got to it (patches welcome). It's small enough, though that I'd suggest starting with an empty version and add things until the compile errors stop. From memory it's a small handful of mem*() and str*() functions, plus the fdtXX_to_cpu and cpu_to_fdtXX byteswap functions - note also that the opencoded example versions of those in the included libfdt_env.h don't always compile to particularly efficient code. It rarely matters in userspace, but can in limited firmware environments (although probably not for something running a firmware as heavyweight as UEFI). If your environment has byteswap functions, it's generally best to #define the libfdt ones to redirect to them.

Is there any specific issue with this commit that makes it unsuitable?

Complexity. The sample libfdt_env.h is already larger than I'd like to handle differences between more-or-less POSIXish userspace environments. I don't want to add anything more to it for environments that aren't even in that category.

[dgibson]

Btw, the test failure is due to some long-deprecated functions being removed from swig. If you rebase, it should go away.

[bluca]

libfdt_env.h doesn't have to provide much - the userspace version has much more than is actually needed, simply because pulling in the system headers we need includes much more than we actually need. libfdt is specifically designed to be compilable in unusual and limited environments, and libfdt_env.h is the mechanism. libfdt has deliberately minimal dependencies on the external environment and all of them come via libfdt_env.h - we never include system headers except indirectly via libfdt_env.h.

If I understand correctly, this serves two functions: include system headers, and provide those macros/inlines. Shouldn't this be split then, so that the system headers are in one header, and the fdt code in another? That way we can override the system headers include, but not reinvent the code that is already provided in those macros/inlines. Does that make sense?

The exact things it needs to provide should be documented somewhere, but alas we never got to it (patches welcome). It's small enough, though that I'd suggest starting with an empty version and add things until the compile errors stop. From memory it's a small handful of mem*() and str*() functions, plus the fdtXX_to_cpu and cpu_to_fdtXX byteswap functions - note also that the opencoded example versions of those in the included libfdt_env.h don't always compile to particularly efficient code. It rarely matters in userspace, but can in limited firmware environments (although probably not for something running a firmware as heavyweight as UEFI). If your environment has byteswap functions, it's generally best to #define the libfdt ones to redirect to them.

Note that we are not going to compile fdt itself. There's no need to do so: the normal static library as shipped in Ubuntu or Fedora works just fine when included in an EFI binary. The only issue is the public header fdt.h that the application has to include, since via fdt_env.h it pulls those two glibc headers at EFI binary build time, which has conflicting definitions for some symbols. But libfdt.a itself is unchanged and just linked in from the normal linux userspace build as shipped in all major distros.

[dgibson]

libfdt_env.h doesn't have to provide much - the userspace version has much more than is actually needed, simply because pulling in the system headers we need includes much more than we actually need. libfdt is specifically designed to be compilable in unusual and limited environments, and libfdt_env.h is the mechanism. libfdt has deliberately minimal dependencies on the external environment and all of them come via libfdt_env.h - we never include system headers except indirectly via libfdt_env.h.

If I understand correctly, this serves two functions: include system headers, and provide those macros/inlines. Shouldn't this be split then, so that the system headers are in one header, and the fdt code in another? That way we can override the system headers include, but not reinvent the code that is already provided in those macros/inlines. Does that make sense?

They're not really different purposes. libfdt_env.h provides all the (few) external dependencies of libfdt. Those can come from importing system headers, or from headers of the project libfdt is being embedded within, or from direct inline implementations. The byteswaps are considered external dependencies, since ideally they would be provided from outside, with just renaming alises in libfdt_env.h.

However byteswaps using libc helpers are surprisingly tricky to make portable, since the details and exact location of the right functions isn't well specified by POSIX. So we made the choice to instead open code the byteswaps, although this results in poorer code. When embeddeding in known environments with existing byteswap helpers, it's generally best to use them (for stage 1 bootloaders running in extremely limited RAM on ARM or RISC-V SoCs, this can really matter).

The exact things it needs to provide should be documented somewhere, but alas we never got to it (patches welcome). It's small enough, though that I'd suggest starting with an empty version and add things until the compile errors stop. From memory it's a small handful of mem*() and str*() functions, plus the fdtXX_to_cpu and cpu_to_fdtXX byteswap functions - note also that the opencoded example versions of those in the included libfdt_env.h don't always compile to particularly efficient code. It rarely matters in userspace, but can in limited firmware environments (although probably not for something running a firmware as heavyweight as UEFI). If your environment has byteswap functions, it's generally best to #define the libfdt ones to redirect to them.

Note that we are not going to compile fdt itself. There's no need to do so: the normal static library as shipped in Ubuntu or Fedora works just fine when included in an EFI binary. The only issue is the public header fdt.h that the application has to include, since via fdt_env.h it pulls those two glibc headers at EFI binary build time, which has conflicting definitions for some symbols. But libfdt.a itself is unchanged and just linked in from the normal linux userspace build as shipped in all major distros.

Oh, I see. That sounds... extremely fragile. You're essentially building libfdt and the programs using it against different libc environments. More specifically that means the pieces of libfdt in libfdt.a are being compiled against a different libc environment than the inline parts in libfdt.h. Evidently you're getting away with this - probably because libfdt's dependencies are so minimal there's unlikely to be a conflict of substance. AFAICT there's no guarantee that this should work at all though.

[bluca]

Oh, I see. That sounds... extremely fragile.

It's actually not, and has been working very well for many years. After all a static archive is just a collection of the object files, the only thing that matters is that compatible external symbols are provided, and that's very easy to do since it's the same symbols for every other library as well, and they match the userspace versions. Works nicely, distribution is trivial, and maintenance costs are minimal.

[bluca]

Closing since the intended workflow is to have a local minimal libfdt_env.h