fix several printf-related warnings

[mazunki]

Cleaning up more warnings, this time related to printing. This also introduces the Expectsf and Ensuresf functions which act similar to the old Expects and Ensures assertion functions, but take a condition and the format specifier as different arguments. I have introduced a basic example of its usage in src/musl/mmap.cpp: Ensuresf(false, "hello {}", 42); will error immediately.

I'd like to update all prior uses of Expects to separate the condition from the message, but I believe it would be cleaner to do this in a later PR. After that we could potentially merge the suffixed version into a single function. Related: What is the semantic difference between Expects and Ensures? They behave identically.

On a similar note, we currently have util/logger.{cpp,hpp} with the Logger class. I believe we could move the infof, warnf and errorf functions from vmbuild/vmbuild.cpp into a namespace for the purpose of logging? Seems like it would be useful to have this in several places. The INFO(namespace, ...) could just be defined local to each file. Is this something we want?

[mazunki]

This change exposes an implicit narrowing cast, which the last commit addresses. They might have been in the logs earlier, but it wasn't until this change where I noticed it.

[mazunki]

This is ready for merge I believe. It seems we have api/info for the purpose of logging as I suggested. I feel it deserves an overhaul to use std::format and expose namespaced functions. It's not obvious that they refer to kprintf() unless one inspects their implementation.

[torgeiru]

Running ./test.sh gave me issues:

In file included from /tmp/nix-build-unittests-dev.drv-0/src/fs/fat.cpp:18:
In file included from /tmp/nix-build-unittests-dev.drv-0/test/../api/fs/fat.hpp:22:
In file included from /tmp/nix-build-unittests-dev.drv-0/test/../api/fs/filesystem.hpp:21:
In file included from /tmp/nix-build-unittests-dev.drv-0/test/../api/fs/common.hpp:27:
In file included from /tmp/nix-build-unittests-dev.drv-0/test/../api/fs/path.hpp:25:
/tmp/nix-build-unittests-dev.drv-0/test/../api/expects:35:74: warning: unused parameter 'panic_text' [-Wunused-parameter]
   35 | inline void __expect_emit_failure(std::string_view msg, std::string_view panic_text) {
      |                                                                          ^
In file included from /tmp/nix-build-unittests-dev.drv-0/src/net/vlan_manager.cpp:25:
In file included from /tmp/nix-build-unittests-dev.drv-0/test/../api/net/vlan_manager.hpp:22:
In file included from /tmp/nix-build-unittests-dev.drv-0/test/../api/net/vif.hpp:22:
In file included from /tmp/nix-build-unittests-dev.drv-0/test/../api/net/link_layer.hpp:22:
In file included from /tmp/nix-build-unittests-dev.drv-0/test/../api/hw/nic.hpp:21:
In file included from /tmp/nix-build-unittests-dev.drv-0/test/../api/hw/mac_addr.hpp:28:
/tmp/nix-build-unittests-dev.drv-0/test/../api/expects:49:9: error: no matching conversion for functional-style cast from 'std::string_view' (aka 'basic_string_view<char>') to 'std::runtime_error'

Run the test script and fix issues. Will redo test.

[torgeiru]

Also, it prints Syscalls not allowed here. Unexpected call to futex (stub).

[mazunki]

This still needs some work to fix the futex issues.

[mazunki]

It seems like both std::format and std::format_to both rely on futex syscalls (I'm not sure how to actually verify this without testing myself. Do the docs tell me somehow?), which aren't available early on. A statically allocated char-buffer solves this.

All tests are passing now.

Thanks @torgeiru :)

[fwsGonzo]

std::format could potentially be replaced with libfmt with the FMT_OS CMake option disabled. it has nice compile-time format-string support anyway.

[mazunki]

Isn't std::format essentially c++20's version of libfmt? I didn't realize we had it available. Do you prefer that over ::format_to_n?

[fwsGonzo]

They have nearly the same API but one is in the standard library and the other is fast, as usual. It's not slow by any means, but if you can't use it because it uses futex internally, then I wouldn't mind bringing in libfmt. It's a staple default in any modern C++ project anyway.

[mazunki]

Nice. I might look into it tonight/tomorrow. The main downside with the statically sized char-buffer, using std::format_to_n() is that we're limited to 512 characters.

Right now I'm cleaning up some warnings which only show up when running tests.

[mazunki]

Thanks for the suggestion on using {fmt} @fwsGonzo , but I can't seem to find any way to convert from format_strings to printf-type strings. Simply replacing std::format to fmt::format didn't magically just work.

I understand the types should be available at comptime, so in theory this should be possible I believe. I suspect a heap is required if {}-type format specifiers are used at runtime (correct me if I'm wrong), which is why it'd be nice to do the conversion at comptime.

I have pulled in libfmt as a dependency regardless, it's a bit more convenient. Since kprintf() uses a fixed size of 8192 characters, I've used this as a buffer size here too (by making a constexpr attribute on the kernel). There isn't a heap available at this point, so I think this is an acceptable solution.

All tests are passing, and I see no printf-related warnings on my end anymore. Yippie. :3

I believe this can be merged now, unless there anything else to do at this time? In the future it'd be nice to have a proper namespace for warnings, info, debugging including colours, prefixes and so forth (i.e. api/info, but namespaced), but I feel that's out of scope for now. Another thing to do is to remove all cases of old-style printf-specifiers, but that's probably best done slowly over time.

[mazunki]

#include <os>
#include <service>
#include <fmt/core.h>
#include <fmt/compile.h>

consteval std::string constexpr_format(auto fmt, auto&&... args) {
  std::string buf;
  fmt::format_to(std::back_inserter(buf), fmt, std::forward<decltype(args)>(args)...);
  return buf;
}

#define _kfmt(prefix, fmt, ...) do { \
  kprintf("%s%s", prefix, constexpr_format(FMT_COMPILE(fmt "\n"), ##__VA_ARGS__).c_str()); \
} while (0)

void Service::start(const std::string& args) {
  _kfmt("service::", "meow {} world", 42);
  os::shutdown();
}

My suspicion was right, it's actually possible to do this at comptime. Worth noting for a later time: currently we're still stuck with the 8192 limit of the serial console buffer.

[alfreb]

Approving this conditionally: I want us to make 100% sure that we can't convince std::fmt to work for us, without having to add futex. If we do need futex, please post an issue to remove this dependency if / when we get to implementing or stubbing futex.

And thanks for this - "{}" is so much easier to maintain than all the printf specifiers😍

[alfreb]

These should live in api/util/ somewhere. I think they could be useful more places. See also https://github.com/includeos/IncludeOS/blob/main/api/util/units.hpp

[mazunki]

Agreed. I was planning to move them into api/info in a later PR where I turn it into a proper namespaced library. Do you think I should partially start building that library here?

[alfreb]

Are we sure there's no way of doing the same without adding another dependency? If we're sure std::fmt absolutely requires futex and fmtlib/fmt doesn't, I'm fine with this. But would much prefer to use the standard if we can.

[mazunki]

The extent of what I changed can work just the same without libfmt. That said, libfmt does have some additional quality of life features such as colours and <fmt/compile.h>. Both std::format() and fmt::format() behave the same when it comes to futexes, and both std::format_to() and fmt::format_to() work without it.

If you want I can revert the addition of libfmt. Should be rather trivial.

[MagnusS]

Can confirm that this builds and that tests succeed after a rebase on current main.

@mazunki this only needs the tracking issue requested by Alfred, then it should be ready to be merged:

Approving this conditionally: I want us to make 100% sure that we can't convince std::fmt to work for us, without having to add futex. If we do need futex, please post an issue to remove this dependency if / when we get to implementing or stubbing futex.

[mazunki]

Yep, I opened an issue about it.

[MagnusS]

Thanks!