Remove log_str() functions and convert their log_signal() users t…

[rocallahan]

…o return std::string

What are the reasons/motivation for this change?

This is a small but easy step towards removing the log_id_cache. See issue #5210.

Explain how this is achieved.

Use std::string instead of char* to simplify lifetime issues.

[QuantamHD]

@widlarizer would you mind taking a look at this? @rocallahan is with us.

[widlarizer]

My idea around this is that we have a couple things to juggle at once. The core idea is that we don't want to ever touch anything like the weird caches/buffers, but we also have external plugin developers whose code we'd be breaking if we break the interface. drivertools.h is exposed externally so we should prefer to tag these methods deprecated with a convenient comment telling plugin devs what to do instead than change their signature. Ideally, I guess we should be able to pivot to .str() methods and have log_ functions redirect to them (using deprecated but unchanged log_str). If we don't want methods for some reason, we should make new functions that return std::string and rename them to fmt_. It never made sense to have some log_ functions that log things and others that don't and just return a char*.

@jix thoughts?

[jix]

I'm all for getting rid of raw char * use throughout our APIs, but I'm not a fan of having to use extra .c_str() everywhere when constructing log messages.

I'd also be in favor of ditching printf style formatting entirely, using e.g. https://fmt.dev instead (or any other alternative that doesn't require much boilerplate when constructing log messages and supports formatting arbitrary types). This would mean all Yosys types could be used directly as format arguments with no need to manually convert them to strings.

I don't think fixing the space leaks should be blocked on us deciding what exactly we're going to do here, so we should merge some form of this PR now. For that, I mostly agree with what @widlarizer wrote, i.e. I'd want to avoid a sudden breaking change without a deprecation period and I'd want to avoid having some log_ functions return char * and some std::string.

What I wouldn't do right now, though, is deprecate the log_id_cache / log_str based functions. With that I'd wait until we have an alternative to the printf style formatting in place, so that we don't end up updating all log message formatting twice (apart from where it's necessary due to critical space leaks).

[rocallahan]

I'd also be in favor of ditching printf style formatting entirely, using e.g. https://fmt.dev/ instead (or any other alternative that doesn't require much boilerplate when constructing log messages and supports formatting arbitrary types). This would mean all Yosys types could be used directly as format arguments with no need to manually convert them to strings.

Yeah, I thought about this approach and mentioned it here:
#5210 (comment)
The main downside of fmt is that their printf-style formatter doesn't statically check format strings against the type of their arguments. E.g.

fmt::printf("Hello %d!\n", "abc");

produces no warnings; instead you get an exception at runtime. That would be a regression from the current situation where those strings are checked at compile time using the compiler's "printf format" attribute. (And I expect fixing that within the constraints of C++17 would be very hard... otherwise fmt would have done it.)

[QuantamHD]

@rocallahan In C++20 fmt::printf does check the arguments so all you have to do is build Yosys in C++20 mode as another CI pass to get the compile time checks. That prevents users from having to actually upgrade to c++20 while keeping the codebase clean. That's what I did in OpenROAD to solve this problem.

[rocallahan]

@rocallahan In C++20 fmt::printf does check the arguments

Are you sure about that? Because I'm not seeing it. (clang 20, fmt 11.1)

roc@fedora:~$ cat ~/tmp/test.cc
#include <fmt/printf.h>
int main() {
  fmt::printf("Hello %d!\n", "abc");
}
roc@fedora:~$ clang++ -std=c++23 -lfmt -o ~/tmp/test ~/tmp/test.cc && ~/tmp/test
terminate called after throwing an instance of 'fmt::v11::format_error'
  what():  invalid format specifier
Aborted (core dumped)

[rocallahan]

But it's a good idea anyhow... whether or not fmt does static checks with C++20 or C++23, I could probably write something that does and we could have that in CI. @jix does that sound good to you?

One question: would you want '%s' applied to a const RTLIL::IdString & to automatically apply the skip-first-character logic of log_id()/unescape_id()? Or do you want to continue having something explicit to trigger that conversion (possibly a dedicated conversion specifier, e.g. %#s)?

apart from where it's necessary due to critical space leaks

The most critical leak is in SatGen::importSigSpecWorker() so I'll submit another PR to fix that directly by calling unescape_id(...).c_str() instead of log_id() where necessary.

[QuantamHD]

@rocallahan In C++20 fmt::printf does check the arguments

Are you sure about that? Because I'm not seeing it. (clang 20, fmt 11.1)

roc@fedora:~$ cat ~/tmp/test.cc
#include <fmt/printf.h>
int main() {
  fmt::printf("Hello %d!\n", "abc");
}
roc@fedora:~$ clang++ -std=c++23 -lfmt -o ~/tmp/test ~/tmp/test.cc && ~/tmp/test
terminate called after throwing an instance of 'fmt::v11::format_error'
  what():  invalid format specifier
Aborted (core dumped)

I think you've used the wrong format specifier should be fmt::printf("Hello {:d}!\n", "abc") https://fmt.dev/11.1/api/#compile-time-checks

Ah I guess maybe I'm wrong it looks like the compile time checks only work on fmt::format and fmt::print not the printf function.

[rocallahan]

I'll submit another PR to fix that directly by calling unescape_id(...).c_str() instead of log_id() where necessary.

See PR #5217.

[jix]

I hadn't considered using fmt for printf style formatting but was talking about migrating to its native formatting.

If there's an easy way to use fmt to make the existing printf style formatting support std::string arguments directly, so that we can change the return types of the log formatting functions to std::string without requiring changes at most of their call sites, that would seem like the best way of getting rid of a lot of raw char * use and the log_id_cache. That is, as long as we have some reasonable way to still statically type check the printf style formatting, which is the one missing piece afaict.

There might be the occasional call that doesn't pass the output directly to a formatting function, so it would still be a breaking change to change the return types of log_* to std::string, but I think the calls that are neither passing the result to a formatting function, nor cast to a std::string should be rare enough that we could go ahead with such a change anyway. @widlarizer what do you think?

Even if we can make the printf style formatting work with std::string and use that to get rid of the log_id_cache, I think there are advantages of moving to native fmt formatting, so that still seems worthwhile to me. If we can make printf style formatting behave, though, I don't see an issue if we only use native fmt primarily for new code and migrate existing printf style formatting to it whenever that turns out to be more convenient.

[rocallahan]

If there's an easy way to use fmt to make the existing printf style formatting support std::string arguments directly, so that we can change the return types of the log formatting functions to std::string without requiring changes at most of their call sites, that would seem like the best way of getting rid of a lot of raw char * use and the log_id_cache. That is, as long as we have some reasonable way to still statically type check the printf style formatting, which is the one missing piece afaict.

I don't know about "easy" but I've been working on this and I'm pretty sure it's possible. As @QuantamHD suggested, the static checking would only happen when compiling with -std=c++20 or later.

Migrating all printf-style formatting to fmt-style formatting would be a huge change. I think it would be good to get rid of log_id_cache (or most uses of it) before doing a migration like that.

[widlarizer]

While we could go with this approach, what I gather is that we would be risking plugin breakage for an interim solution, right? I think we should save our firepower for internally converting to fmtlib (printf-style first) and marking current formatting functions deprecated. That seems like the most efficient way of going at this

[rocallahan]

I think we should save our firepower for internally converting to fmtlib (printf-style first) and marking current formatting functions deprecated.

I'm not exactly sure what you mean by "current formatting functions" here. If you mean "deprecate stringf() and log()" that's a huge amount of churn for not much gain.

I think the most important thing is to merge PR #5243 so that log() accepts const std::string& parameters directly. That should be source-compatible with existing plugins, since as you can see in that PR, it is source-compatible with the existing Yosys codebase.

In this PR I would deprecate log_str() and drivertools' log_signal(). I would convert the log_signal() implementations to signal_str() functions that return std::string, convert Yosys to use those, and make the log_signal() implementations call signal_str() and call log_str() on the result. If we do that after merging PR #5243 we don't have to add .c_str() anywhere.

[widlarizer]

I meant a soft deprecation where we keep log_... and stringf however long we want (forever?), mark them deprecated so users get warned about that (and can suppress the warning if they want), and internally convert to fmtlib. In the case of loggin, we can stick to providing the current interface for whoever wants it

[rocallahan]

Ok. I think you should take PR #5243 because it provides most of the benefits of fmtlib immediately and without churn.

[jix]

@rocallahan, with your logging PRs merged (#5304 #5243), we should now be in a position where we can change the char * returning log_... functions to return std::string or std::string_view, which in turn should allow us to get rid of the cache and the space leak caused by it.

It shouldn't require any changes for the vast majority of uses as argument for logging or string formatting. Any use that already converted the char * to std::string or std::string_view will also remain working. Only code that requires the result to be a char * would need changes, but we discussed this again, now that the other PRs are in place, and given that the fallout from that is small enough and easy enough to fix, and most likely only affects code that should be refactored anyway, we agreed that we should go ahead with it.

This PR does need to be updated, though, since it still introduces (now unnecessary) calls to .c_str(). I'd also be fine with updating all the users of the string cache and removing the cache itself all in a single PR, but doing it in separate PRs is also no problem.

[rocallahan]

This PR does need to be updated, though, since it still introduces (now unnecessary) calls to .c_str().

Right. Updated as requested. I'll fix the rest in another PR.