YosysHQ / yosys

Yosys Open SYnthesis Suite
https://yosyshq.net/yosys/
ISC License
3.32k stars 859 forks source link

´Use g++ and clang++ instead of gcc and clang as C++ compilers #4234

Closed RCoeurjoly closed 4 months ago

RCoeurjoly commented 4 months ago

This would solve the CXX and LD points of https://github.com/YosysHQ/yosys/issues/2011. LD is also changed to g++ when appropriate.

All pipelines pass in my fork (https://github.com/RCoeurjoly/yosys/actions).

RCoeurjoly commented 4 months ago

It seems that clang++ or g++ can be used as the linker, specially if sanitizers are used: https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#usage.

I think it is not viable to use ld, because -rdynamic is supported in g++ and clang++ and not in ld, if I am not mistaken. When I set LD = ld or remove $LD, I get the following error: ld: unrecognised option: -rdynamic

whitequark commented 4 months ago

I think it is not viable to use ld

That is possible. In that case you should remove $(LD) entirely and use $(CXX) instead to link.

RCoeurjoly commented 4 months ago

I think it is not viable to use ld

That is possible. In that case you should remove $(LD) entirely and use $(CXX) instead to link.

Done. I had to change $LD for $CXX in passes/techmap/Makefile.inc, otherwise we would get the same linking error

ld: unrecognised option: -rdynamic

whitequark commented 4 months ago

Looks better, thank you. Now that we don't have $(LD), we should not use $(LDFLAGS) either; I propose $(LINKFLAGS). Similarly, instead of $(LDLIBS) we can use $(LIBS).

It looks like we have a bunch of $(filter-out -rdynamic) clauses that can probably be simplified by not adding -rdynamic by default in first place, but I'll take another look at that later, and it doesn't strictly speaking have to be a part of this PR.

RCoeurjoly commented 4 months ago

Looks better, thank you. Now that we don't have $(LD), we should not use $(LDFLAGS) either; I propose $(LINKFLAGS). Similarly, instead of $(LDLIBS) we can use $(LIBS).

It looks like we have a bunch of $(filter-out -rdynamic) clauses that can probably be simplified by not adding -rdynamic by default in first place, but I'll take another look at that later, and it doesn't strictly speaking have to be a part of this PR.

Done. I had to fix it in other Makefiles too.

Now two tests fail

I have reproduced the error locally by running: cd tests/various bash run-test.sh

I get the following error:

make: *** [run-test.mk:456: async.sh] Error 127

I am a bit lost as to how to debug that failure.

povik commented 4 months ago

Now two tests fail

Those happen to be the only CI jobs which run the testsuite (through make test), the other CI jobs build the binaries but don't run make test.

Looks like the fault is in tests/various/plugin.sh because yosys-config needs updating, see misc/yosys-config.in.

RCoeurjoly commented 4 months ago

Looks good to me, thank you a lot! The Makefiles are not perfect but they are at least not blatantly misleading anymore, so we are getting somewhere!!

Thank you for your patience and handholding!