Missing '{' in llvm-cbe.cpp:401 ?

makise-homura commented 3 years ago

I have LLVM 9.0.1, and got a fauliure building llvm-cbe.c in this place around line 401:

#if LLVM_VERSION_MAJOR > 10
  if (mc::getExplicitRelaxAll()) {
    if (codegen::getFileType() != CodeGenFileType::CGFT_ObjectFile)
#elif LLVM_VERSION_MAJOR == 10
  if (RelaxAll) {
    if (FileType != CodeGenFileType::CGFT_ObjectFile)
#else
    if (FileType != TargetMachine::CGFT_ObjectFile)
#endif

Note that in cases where LLVM_VERSION_MAJOR >= 10, there is opening { in each block, but for else-case (LLVM_VERSION_MAJOR < 10) there is no {, so compiler breaks there. I put { there, like this:

    if (FileType != TargetMachine::CGFT_ObjectFile) {

and everything worked. I wonder, if this is correct? It looks like an obvious bug, but I'm not sure why then it is not found and fixed after all. (I guess many of users of llvm-cbe use LLVM 9 and older, but why didn't they get this error?)

P.S. I use an Elbrus CPU based PC, and expected build to fail because of some architecture incompatibility, but once I get into sources, I found the reason to be pretty architecture independent. To my great surprise, it worked perfectly after this fix, and even successfully passed all unit tests.

P.P.S. If you wish, I may format this fix as pull request, if my guess about this fix is correct.

hikari-no-yume commented 3 years ago

I think this is a duplicate of https://github.com/JuliaComputingOSS/llvm-cbe/issues/119.

You are free to submit a PR to fix this if you want to, but please note that we are not claiming to support LLVM versions older than 10.0.

hikari-no-yume commented 3 years ago

(With that said, chances are that things will mostly or fully work if you get it to build.)

makise-homura commented 3 years ago

Right, I've got the idea. Still I hope if there's no problem in current version with LLVM 9.0.1 at least, I guess there's no need to forbid the ability to build llvm-cbe under older versions of LLVM (of course, it's at own risk of those who uses versions < 10.x). Created PR #131.

hikari-no-yume commented 3 years ago

I would guess this is a recent breakage and LLVM 9 otherwise works fine.

makise-homura commented 3 years ago

As long as #131 is merged, everything seems to be ok with LLVM 9. Thanks!

JuliaHubOSS / llvm-cbe

Missing '{' in llvm-cbe.cpp:401 ? #130