[WIP] Detect presence of flockfile

[mvastola]

@vitaut,

Wanted to continue the discussion from #4646 since that's a closed issue..

I think declaring functions without actually implementing them is really broken and should be fixed but I'm open to a PR to set FMT_USE_FLOCKFILE to 0 on newlib, provided that it's easy to detect.

@vitaut thanks for your understanding on this. I truly appreciate what you're saying, and it makes sense, but from what I've seen (and from the logistics I anticipate would be involved in not doing so), separating the declarations and implementations of low-level standard library functions in such a way that the former can't depend on the latter is SOP for bare-metal targets. (If you have any resources that suggest otherwise, I'm very open.)

A few clarifications:

How strongly wedded are you to using the presence of newlib specifically as the deciding factor in setting FMT_USE_FLOCKFILE?

IMHO, the ideal solution would be to do something along the lines of what I've done in this PR, which directly tests if flockfile can be linked, especially if you're now okay making a small addition to the CMake script. This involves as little fuss as possible and is 100% accurate, baring a mismatch in compile options between the compilation of this library and the software it is included in.

I'm realizing, however, that while your stated condition was that it be "easy to detect" and this implementation is certainly simple, it's possible you wanted me to just do this via a macro. I wanted to double check since you expressed reticence about doing this in CMake previously.

---

Unfortunately, if we restrict ourselves to macros and/or detecting newlib, the accuracy of testing if flockfile works/links drops significantly:

It seems there is an official macro to test the presence of newlib (aptly named __NEWLIB__), but as I mentioned previously, the use of newlib isn't dispositive of flockfile being unimplemented. Newlib is just one libc implementation popular with bare-metal targets (others include picolibc and LLVM libc). Further, some bare-metal platforms do, in fact, support filesystems (and, thus, flockfile).

If you're not convinced about using CMake to do the check, I'd be happy to address any concerns you have and figure out how to obviate them.

For sake of completeness, I've also included several macro-only solutions I've investigated here, with my assessment of each. As mentioned, they unfortunately all have (often major) downsides.

1. Testing for a bare-metal target with #if __STDC_HOSTED__ == 0.
   Downside: This depends on the user or platform toolchain passing -ffreestanding to the compiler, which isn't always done.
2. Testing for the absence of any common OSes. For example: #if !defined(_WIN32) && !defined(__linux__) && !defined(__APPLE__) && !defined(__unix__) ...
   Downside: this is brittle and would requires an exhaustive list of OSes with filesystems. Additionally, there are quite a few RTOS OSes where filesystem support is an optional feature.
3. Using macros make an educated guess as to if a target is bare-metal. For example: #if (defined(__ELF__) && !defined(__linux__) && !defined(__unix__)) || defined(__ARM_EABI__)
   Downside: While this would likely be more accurate than the previous two at identifying bare-metal targets, it still can't tell if its being used on a bare-metal target with a filesystem, and is likely to miss many bare-metal targets anyway.
4. Testing if #include <dirent.h> works. While newlib doesn't have any macros to indicate filesystem support, one thing it does have is a default sys/dirent.h containing #error directive. If <dirent.h> is included and sys/dirent.h is not masked by a file of the same name in the platform's headers containing a real implementation, a compile-time error occurs.
   Downside: this only works on newlib, plus there's no way to use this indicator without using CMake to assist, because the compiler will only know if it has an #error directive by #includeing it, at which point you've broken compilation.

[mvastola]

@vitaut, If it helps, here is a Dockerfile (like the one in #4646) showing this isn't limited to newllib. This demonstrates the identical problem with LLVM's libc implementation, based on code I am proposing we test-compile with this PR.

(Click to expand `Dockerfile` source)

# syntax=docker/dockerfile:1
FROM ubuntu:noble

# Lastest release of official "Arm Toolchain for Embedded"
ADD --unpack=true https://github.com/arm/arm-toolchain/releases/download/release-22.1.0-ATfE/ATfE-22.1.0-Linux-x86_64.tar.xz /usr/local
ENV TOOLCHAIN_DIR="/usr/local/ATfE-22.1.0-Linux-x86_64"
ENV TOOLCHAIN_BIN_DIR="${TOOLCHAIN_DIR}/bin"

ENV PATH="${TOOLCHAIN_BIN_DIR}:$PATH"
ENV SRC_DIR=/src

RUN mkdir -p "${SRC_DIR}"

ADD --chmod=0755 <<EOF "${SRC_DIR}/build-and-run.sh"
#!/bin/bash

set -euo pipefail

CFLAGS=(
  --std=c23
  --target=armv7-unknown-none-eabi -mfpu=none # generic defaults so the compiler can pick a set of runtime libraries
  --config=\${TOOLCHAIN_BIN_DIR}/llvmlibc.cfg -Wl,-Tllvmlibc.ld # use llvm libc\'s runtime libraries and linker script
  -static # no dynamic linking available because no dynamic linker present (nor fs to load shared libs from) on bare-metal
  -nostartfiles # by default, the llvm linker looks for (the non-existent) crt0.o; we need to specify our own because the real file is named like libcrt0.a
  -lcrt0-semihost -lsemihost # choose the libcrt0.a for semihosting, which provides support for stdin/out
)

set -x
cd "\${SRC_DIR}"

! test -f "test" || rm -f test

"${TOOLCHAIN_BIN_DIR}/clang" "\${CFLAGS[@]}" -o test test.c
exec ./test

EOF

ADD <<EOF "${SRC_DIR}/test.c"
#include <stdio.h>
#if __has_include(<newlib.h>)
#  include <newlib.h> // include __NEWLIB__ macro, if it exists
#endif

#if __NEWLIB__
#  warning "using newlib"
#endif
#if __LLVM_LIBC__
#  warning "using llvm libc"
#endif

int main() {
  flockfile(stdout);
  funlockfile(stdout);
  printf("Test complete!\n");
  return 0;
}
EOF

WORKDIR "${SRC_DIR}"
CMD ["./build-and-run.sh"]

To use, write the contents above to a file named Dockerfile in an empty directory and (within that directory) run:

docker build -t mvastola/fmt-test-flockfile-poc-llvm-libc . && docker run --rm mvastola/fmt-test-flockfile-poc-llvm-libc

Expected output is:

+ /usr/local/ATfE-22.1.0-Linux-x86_64/bin/clang --std=c23 --target=armv7-unknown-none-eabi -mfpu=none --config=/usr/local/ATfE-22.1.0-Linux-x86_64/bin/llvmlibc.cfg -Wl,-Tllvmlibc.ld -static -nostartfiles -lcrt0-semihost -lsemihost -o test test.c
test.c:10:4: warning: "using llvm libc" [-W#warnings]
10 | #  warning "using llvm libc"
|    ^
1 warning generated.
ld.lld: error: undefined symbol: flockfile
>>> referenced by test.c
>>>               /tmp/test-1e0caa.o:(main)

ld.lld: error: undefined symbol: funlockfile
>>> referenced by test.c
>>>               /tmp/test-1e0caa.o:(main)
clang: error: linker command failed with exit code 1 (use -v to see invocation)

[mvastola]

So, I think I was able to come up with the beginnings of a headers-only way to do this, if you feel strongly about approaching it that way.

Let me know if you prefer this approach. While this wouldn't be my preference, I wanted to put it out there as an alternative approach if you're not comfortable with the one in this PR. (If that's not an issue, no need to bother with this as it's a more complex and slightly more fraught solution.)

Basically this alternate approach turns f(un)lockfile info weak symbol references, thereby avoiding linker errors. (NB: This is untested/not finalized, but gives a good idea of the implementation.)

There are some downsides/implications, that I'll list here for your consideration:

• This solution is only coded for GCC and Clang, though it potentially can be tuned to work on any compiler/target combination supporting __attribute__((weak)). (We can potentially use __has_attribute but not sure what support is like across other compilers/versions or what is returned if the compiler generally supports the attribute but the binary file format doesn't.)
• This approach requires use of wrapper functions (which I've named fmt_f(un)lockfile) rather than using f(un)lockfile file directly, as seen in the diff I linked in this comment.
• This approach can't detect if f(un)lockfile exists at compile time (since obviously the linking step comes after compilation). This implementation adds fmt_f(un)lockfile functions that are no-ops if there aren't any linked-in f(un)lockfile methods for to call. For something like file_print_buffer there would need to be a runtime check to determine if the locking specialization of file_print_buffer should be used. (Fortunately both file_print_buffer specializations are subclasses of buffer with no other public functions, so such a change would be pretty seamless.)
• It seems you were making something of a habit of using a type parameter in lieu of FILE in format-inl.h. I'm not sure of the reasoning behind this (if it was purely stylistic, there's no issue), but the approach here requires forward-declaring f(un)lockfile with extern "C" linkage, so a template isn't an option: the type FILE* has to be used directly.
• At some point in the past, the f(un)lockfile methods defined in format-inl.h to wrap Windows' _(un)lock_file methods were behind a #ifdef _WIN32 (or similar) that has since been removed. It's possible we may need to place them behind such a conditional again to avoid a conflict between the Windows wrapper and the weak symbol.

[vitaut]

Thanks for the PR. CMake is the recommended build systems but not the only one {fmt} users rely on. So I still suggest putting the check in the code and relying on __NEWLIB__ macro. If there is other similarly broken system, we can generalize the check similarly to what we do for other FMT_USE_* macros. Weak symbols is an interesting idea but will likely be more complex as you outlined in the implications of this approach.

[mvastola]

(Updated with some added info.)

Thanks for the PR. CMake is the recommended build systems but not the only one {fmt} users rely on.

@vitaut, you mentioned this in the issue thread as a reason not to do the check in CMake, but I didn't understand this point then either; I think there might be a misunderstanding here, but I'm not sure on whose part it is:

If we implemented autodetection in CMake (a la this PR as it is currently), users who don't use CMake simply wouldn't be able to take advantage of the macro value being determined for them, in the same way that they can't take advantage of any of the other defines/build/compile options that CMakeLists.txt and CMake itself automagically determines. Merging this PR would have no impact whatsoever on people not using CMake, who would remain free to define the FMT_USE_FLOCKFILE macro manually without any change in its effect. I think there might be something I'm missing because to me, this approach seems to be the safer/default option, especially given your (understandably) conservative approach to making changes.

I guess I just don't understand why the fact that some users don't use CMake would counsel against using CMake to much more accurately guess how to set the macro for people who do use CMake, particularly when it's the recommended build system and using it already removes the need to specifically set many other compile options.

If we wanted to use the __NEWLIB__ macro or similar as a fallback for non-CMake users, that could be done very easily, though honestly I think using __NEWLIB__ anywhere is inadvisable, specifically because it will result in a non-negligible frequency of false negative guesses that file locking isn't available. I'm also a bit hesitant because switching to using it is arguably a breaking change for the unknown number of existing users who use newlib/LLVM libc and have platform support for filesystems/file locking. (Such users are currently using fmt just fine without needing to define any macros.)

So I still suggest putting the check in the code and relying on __NEWLIB__ macro.

Is there any reason why we couldn't/shouldn’t compromise by using the CMake compile check for CMake users, and use __NEWLIB__ (assuming that remains your preference) for everyone else?

If there is other similarly broken system, we can generalize the check similarly to what we do for other FMT_USE_* macros.

Could you elaborate on this? As far as I can tell, these are set (in include/fmt/base.h) based on compiler builtins and macros, but, of course, the entire reason this has been such a to-do is that you can't determine the availability of f(un)lockfile with any great accuracy from headers alone.

If we implemented this headers-only, we would have to do it by detecting the presence of specific libc implementations associated with bare-metal/embedded devices. The ultimate result is that we'd be disabling f(un)lockfile by default on nearly all microcontrollers and embedded devices (and, worse, many devices that are neither).

This is especially problematic when you consider that LLVM's libc as I mentioned has the same problem with a declared but not necessarily defined f(un)lockfile, because it is widely used in other scenarios well beyond just microcontrollers/embedded devices and likely including some existing fmt users; doing this would disable file locking for them as well.

I wonder if part of the disconnect could be that you're focusing on these cases as being on "broken systems".. While I certainly agree that declaring functions without implementing them is far less than ideal and (obviously) has the potential to cause annoying issues (this one being a case in point), whether they're broken/poorly designed is kind of beside the point: this flaw is common to any OS or device-independent libc implementations. Compliance with standards like POSIX requires that implementations declare the standard set of methods, while their definitions/availability always inherently depend on the OS and hardware capabilities.

Weak symbols is an interesting idea but will likely be more complex as you outlined in the implications of this approach.

Agreed, unfortunately. :-\

[vitaut]

The issue isn’t that some users don’t use CMake, it’s that putting the logic in CMake makes the behavior depend on the build system rather than the code. fmt is meant to be build-system agnostic, and we try to keep configuration in code so all users get consistent defaults.

Using something like __NEWLIB__, possibly combined with a more specific platform check is a reasonable fallback. Users on platforms where the heuristic is wrong can still override the macro explicitly, which keeps behavior predictable and uniform across build systems.

We already spent way too much time on this esoteric issue.

[mvastola]

I know this has taken longer than it should, and I don't want to prolong this, nor butt heads.

I think we have some fundamentally different philosophies/priorities and issue happens to hit at the heart of those differences. I know this is your project, but at the same time I still feel a sense of responsibility/ownership for the code I push and I've been struggling because I fear the fix you are proposing to this relatively esoteric issue will negatively impact the userbase beyond those whom this issue actually does/should affect. And I don't want to make a contribution I can't be proud of, or -- at the very least -- confident in. I guess the bottom line is that, despite trying, I can't convince myself using __NEWLIB__ is a "reasonable fallback" and I have been hoping there was knowledge/information/perspective we could exchange to get on the same page there.

The value the macro needs to be set to has at best a tertiary relationship with any particular libc implementation. f(un)lockfile isn't guaranteed for an entire class of target platforms (i.e. bare-metal/embedded), and newlib is just one libc implementation popular on such platforms. Even if we ignored the fact you can still have f(un)lockfile on platforms with newlib, using __NEWLIB__ to guess the macro value only addresses a fraction of the platforms where those methods are unavailable. My entire reason for pursuing this was that because fmt boasts portability and can easily avoid a compile error by configuring itself not to use f(un)lockfile, it should do so. Solving this with an imperfect, double-edged substitute like __NEWLIB__ when there is an exceedingly simple and near-perfect solution doesn't feel like an improvement. And the fact that you actually can have f(un)lockfile on embedded platforms means such users will suddenly start experiencing intermittent and hard-to-trace race conditions due to the fact they chose to build with newlib. That's the antithesis of "uniform and predictable". At least as it stands with the current code, users affected by this problem get a compile error and will know something is wrong, even if that places use of this library out of reach for some that don't know how to fix it.

In any case, I regret the extent to which I facilitated the focus on newlib. I am happy to provide more info or further explanation if it will help, but I'm not trying to waste your time, so please tell me if you think discussing this further would be helpful.

While I totally concur that consistent config defaults is a worthy goal for settings/preferences, the value this macro should be set to is inherently and inextricably dependent on the build and run environments, rather than anything that can be determined from within library's code at compile-time. As worthy of an ideal that consistent defaults are, I think it's fundamentally outweighed by the implicit/universal expectation by users that their build system (if they choose to use one) handles their software's build configuration for them and doesn't break it in subtle and unexpected ways between versions. The desire to keep configuration consistent shouldn't be taken to such an extreme that it effectively becomes the goal that use of the build system confers no benefit at all.

When someone decides to not use the supported build system for any software, they take up the responsibility of looking at what the pre-packaged build system does and duplicating its functionality. They must expect to need to configure things manually that the build system does automatically. Someone rolling their own build process would also be expected to be on the lookout for changes to distributed build scripts between versions and ensure their own custom build process remains consistent. If a compile option can't be correctly guessed for some portion of the userbase, these users should be the preference to comprise that portion.

As for where we go from here, I honestly don't know. I just can't get behind what you're proposing. If you see some sort of hole in my logic, I'm more than happy to hear you out. If there's something that I said that wasn't clear or accurate, I'd be more than happy to revisit it or look closer. Baring that or some sort of compromise though, I don't want to keep beating a dead horse if we've truly hit an impasse. There won't be any hard feelings from if you choose to make this change on your own, or if you want to just close this PR.

In that case, I'm just sorry this didn't work out.