Fix 'experimental/coroutine' file not found on macOS (#5030)

[selimsandal]

Enables stdc++20, includes coroutine as include <coroutine> if clang_major >= 15 || clang_major == 14 && clang_minor >= 0 && clang_patchlevel >= 6

[wsnyder]

We only require C++20, there's a later test for what's needed for coroutines - so should be added there presumably,

[selimsandal]

Is there a reason c++20 std is not enabled by default? If there is, this check can go under AC_MSG_CHECKING([whether coroutines are supported by $CXX]) I guess?

If not this shouldn't cause any problems with older stdlibs/compilers as far as I can tell.

[wsnyder]

We still want older compilers to work, and because choosing a std that does not match glibc often makes compiling break. Ideally you wouldn't need that flag to use coroutines.

[selimsandal]

Since they moved it from experimental/coroutine to coroutine without enabling c++20 std does not include coroutines. If we switch to c++17 we need "experimental/coroutine" but updated versions of the compiler does not have that.

So I don't see a simple way to make it work with both new and old compilers without enabling c++20. If there is any check I can make before enabling it (if we are going to enable it) let me know, I am not familiar with autoconf.

[wsnyder]

Try instead adding this to the later section

_MY_CXX_CHECK_SET(CFG_CXXFLAGS_COROUTINES,-std=c++20)

[selimsandal]

Added that to coroutine checks, enables c++20 with clang 17.0.6. Tested with multiple modules, seems like it's working.

[wsnyder]

As above we cannot change NEWEST, please try the other flag suggested. Also would really like a config fest instead of looking for a magic clang version which is likely fragile.

[selimsandal]

As above we cannot change NEWEST, please try the other flag suggested. Also would really like a config fest instead of looking for a magic clang version which is likely fragile.

We tried _MY_CXX_CHECK_SET(CFG_CXXFLAGS_COROUTINES,-std=c++20), caused some gcc tests to fail. What should we test to enable c++20?

[wsnyder]

Tests failing. Maybe we should have a configure check instead to try both includes.

[gussmith23]

Bump on this -- encountering this issue as well, would be great to have this merged!

Thanks all :)

[selimsandal]

Bump on this -- encountering this issue as well, would be great to have this merged!

Thanks all :)

In the meantime I added the fork to my homebrew tap and reverted the commit that breaks gcc tests. You can use brew install selimsandal/selimsandal/verilator --HEAD if you need it.

[FanShupei]

I encounter the same problem with clang 17 on Arch Linux. Thanks for your patch now it can compile, but it gives another warning.

warning: unknown warning group '-Wdeprecated-experimental-coroutine', ignored [-Wunknown-warning-option]
   37 | #  pragma clang diagnostic ignored "-Wdeprecated-experimental-coroutine"

It looks like recent clang does not like this pragma. May you consider remove it?

[wsnyder]

@selimsandal (or other) could you please update the patch to use configure.ac to properly discover the include and flags? Then we should be solid for all versions.

[gussmith23]

Is this good to merge?

[wsnyder]

Not ready, see review comments and please update the patch to use configure.ac to properly discover the include and flags? Then we should be solid for all versions.

[mxg]

@wsnyder

Not sure if you saw my message since it was posted to a closed issue. Reposting here.

wsynder: Note #5031 still needs changes to check using configure.ac (see the ticket), perhaps you can help that.

mxg: I'm happy to help, but I need to figure out what you expect. The PR has code in configure.ac to check for and changes in verilated_timing.h. I'm not an autoconf expert, but if you could help me with what you're looking for, I can do the heavy lifting.

[wsnyder]

Thanks for looking into this. Here's some existing code in configure.ac that compiles a program to see if it works:

verilator/configure.ac

Line 361 in 19cccd1

AC_MSG_CHECKING([whether coroutines are supported by $CXX])

You'd want to try the non-experimental, then if fails try the experimental and set a define as to which is found, e.g. set HAVE_COROUTINES in addition to HAVE_EXPERIMENTAL_COROUTINES, then in the verilated/ include files do an #ifdef on HAVE_EXPERIMENTAL_COROUTINES to pull in the appropriate include.

[ethanuppal]

I'm also having this issue:

1. Are there any updates on when this PR will be merged?
2. Are there any known workarounds/"hacky" ways to get around this? Should we build from source and modify the file? I saw the comment above about the homebrew tap.

[wsnyder]

Still waiting on a pull that uses configure.ac.

[ethanuppal]

I also encountered a homebrew (?) error while trying to use the tap (HOMEBREW_MAKE_JOBS=2 brew install selimsandal/selimsandal/verilator --HEAD); my computer (M2) became very hot for an extended period of time and the build subsequently failed. I'm putting the issue here because I can't seem to report it on @selimsandal's fork. There might be an issue in the formula?

==> make install
ruby(37992) MallocStackLogging: can't turn off malloc stack logging because it was not enabled.
ruby(38291) MallocStackLogging: can't turn off malloc stack logging because it was not enabled.
ruby(38320) MallocStackLogging: can't turn off malloc stack logging because it was not enabled.
==> Downloading https://formulae.brew.sh/api/cask.jws.json

Error: undefined method `bottle_specification' for nil:NilClass
/opt/homebrew/Library/Homebrew/formula.rb:2582:in `bottle_hash'
/opt/homebrew/Library/Homebrew/sbom.rb:37:in `create'
/opt/homebrew/Library/Homebrew/formula_installer.rb:834:in `finish'
/opt/homebrew/Library/Homebrew/upgrade.rb:238:in `install_formula'
/opt/homebrew/Library/Homebrew/install.rb:350:in `install_formula'
/opt/homebrew/Library/Homebrew/install.rb:301:in `block in install_formulae'
/opt/homebrew/Library/Homebrew/install.rb:300:in `each'
/opt/homebrew/Library/Homebrew/install.rb:300:in `install_formulae'
/opt/homebrew/Library/Homebrew/cmd/install.rb:297:in `run'
/opt/homebrew/Library/Homebrew/brew.rb:92:in `<main>'
Please report this issue:
  https://docs.brew.sh/Troubleshooting

[selimsandal]

@ethanuppal I didn't change the formula but I think it started failing this week after a brew update. I tried removing unnecessary fields from the formula and reverting the fork to a working version I tried before, but those didn't help.

About your laptop getting hot, I hardcoded the formula to use all cores instead of homebrew's env vars, no need to worry it'll throttle itself if necessary.

Even if Homebrew shows that error it installs verilator as far as I can tell, I'll update you if I find a fix.

P.S: For everyone looking for a temporary fix brew install --HEAD selimsandal/selimsandal/verilator

[paulhuggett]

Forgive me if this is inappropriate but could I suggest that instead of making the choice of include file dependent on the compiler version, __has_include is used to check for the existence of specific include files. So:

# ifdef __has_include
#  if __has_include(<coroutine>)
#    include   <coroutine>
#  elif __has_include(<experimental/coroutine>)
#    include <experimental/coroutine>
     namespace std {
         using namespace experimental;  // Bring std::experimental into the std namespace
     }
#  endif
# endif

This has the advantage that we’re checking the standard library headers directly rather than inferring it from the compiler version. C++20 is still required by <coroutine>, of course.

[wsnyder]

That's a good idea. Will also need the #else for when no has_include.

Can you create a pull with that?

[Frank-Zeyda]

Dear Selim, this reply might not add much but I came across exactly this issue when trying to build the verilator simulation executable for the chipyard RocketConfig Default Example, see: https://chipyard.readthedocs.io/en/1.11.0/Simulation/Software-RTL-Simulation.html#sw-rtl-sim-intro.

The system I am running is Sonoma 14.5 with Apple clang version 15.0.0 (clang-1500.3.9.4) and Xcode version: 15.3.0.0.1.1708646388. These appear to be the latest stable updates at time of writing this post.

After some dabbling, I managed to successfully compile the test executable after rebuilding verilator from master while commenting in:

-#_MY_CXX_CHECK_SET(CFG_CXXFLAGS_STD_NEWEST,-std=gnu++20)
-#_MY_CXX_CHECK_SET(CFG_CXXFLAGS_STD_NEWEST,-std=c++20)
+_MY_CXX_CHECK_SET(CFG_CXXFLAGS_STD_NEWEST,-std=gnu++20)
+_MY_CXX_CHECK_SET(CFG_CXXFLAGS_STD_NEWEST,-std=c++20)

in configure.ac and adding:

 #include <vector>
 
+// Added by Frank Zeyda
+# undef _LIBCPP_VERSION
+
 // clang-format off
 // Some preprocessor magic to support both Clang and GCC coroutines with both libc++ and libstdc++
 #if defined _LIBCPP_VERSION  // libc++

in include/verilated_timing.h. I.e., the pre-processor logic in include/verilated_timing.h seems broken even if making the above change to configure.ac (the above is meant more of a hack/workaround than a fix).

PS: It appears that a robust solution for this issue is still pending—I would be happy to test that once merged into master and greatly appreciate if this problem could be addressed sooner than later (for us MacOS users of verilator ;-).

Thanks a lot!