Fix clang 16.0.4 Compilation on Windows #678
Conversation
|
|
| @@ -25,6 +25,8 @@ include("${CMAKE_CURRENT_SOURCE_DIR}/cmake/utils.cmake") | |||
| include("${CMAKE_CURRENT_SOURCE_DIR}/cmake/options.cmake") | |||
| list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake/modules") | |||
|
|
|||
| set(REMILL_ENABLE_TESTING OFF) | |||
There was a problem hiding this comment.
Move to cmake/options.cmake.
| interpreter | ||
| mcjit | ||
| FrontendOffloading | ||
| nvptxdesc |
There was a problem hiding this comment.
@2over12 thoughts on removing nvptx and wasm from cxx-common?
CMakeLists.txt
Outdated
| # This then causes a windows compilation failure, because of a conflicting definition | ||
| # of 'byte' in one of the windows SDK headers. So as a fix we must append | ||
| # _HAS_STD_BYTE to the compile definitions. | ||
| add_compile_definitions(_HAS_STD_BYTE=0) |
There was a problem hiding this comment.
Is there a way to make this specific to a target?
There was a problem hiding this comment.
Alternatively, can this be turned into a PR to sleigh?
CMakeLists.txt
Outdated
| add_compile_definitions(_HAS_STD_BYTE=0) | ||
| # In another sleigh source file, there is an implicit narrowing conversion. | ||
| # This flag allows implicit narrowing. | ||
| add_compile_options ( -Xclang -Wno-narrowing) |
There was a problem hiding this comment.
Is there a way to make this specific to a target?
There was a problem hiding this comment.
Alternatively, can this be turned into a PR to sleigh?
| @@ -131,7 +165,9 @@ add_library(remill_settings INTERFACE) | |||
|
|
|||
| target_include_directories(remill_settings INTERFACE | |||
| $<BUILD_INTERFACE:${REMILL_INCLUDE_DIR}> | |||
| $<INSTALL_INTERFACE:include>) | |||
| $<INSTALL_INTERFACE:include> | |||
There was a problem hiding this comment.
I think there should be a $<INSTALL_INTERFACE:${CMAKE_INSTALL_INCLUDEDIR}>, given that GNUInstallDirs is included.
There was a problem hiding this comment.
The $<INSTALL_INTERFACE> with the GNUInstallDirs is already implicitly adding when exporting the target, so you should be able to actually remove it from the target.
lib/BC/Util.cpp
Outdated
| case llvm::Instruction::ZExt: { | ||
| auto ret = llvm::ConstantExpr::getZExt( | ||
| /* case llvm::Instruction::ZExt: { | ||
| auto ret = FoldBitCast(c, |
There was a problem hiding this comment.
Why FoldBitCast in a ZExt case?
| @@ -1205,17 +1205,6 @@ MoveConstantIntoModule(llvm::Constant *c, llvm::Module *dest_module, | |||
| moved_c = ret; | |||
| return ret; | |||
| } | |||
| case llvm::Instruction::Select: { | |||
There was a problem hiding this comment.
Why deleted? Does LLVM no longer support these in constant expressions?
There was a problem hiding this comment.
Yeah, they are in the process of gutting the ConstantExpr functionality, this is what's left in LLVM 19: https://github.com/mrexodia/llvm-headers/blob/17bb678153ba1c5c87bb17f6cd36894cb88d5c0d/19.x/llvm/IR/Constants.h#L1101-L1159
| @@ -1229,23 +1218,17 @@ MoveConstantIntoModule(llvm::Constant *c, llvm::Module *dest_module, | |||
| } | |||
| case llvm::Instruction::LShr: { | |||
There was a problem hiding this comment.
Thinking about it, probably a bunch of cases could be merged as follows:
case llvm::Instruction::Foo:
...
case llvm::Instruction::Bar: {
if (auto op = llvm::dyn_cast<llvm::BinaryOperator>(...)) {
... = ConstantFoldBinaryOpOperands(
...
}
}| @@ -300,6 +300,10 @@ function GetLLVMVersion | |||
| LLVM_VERSION=llvm-16 | |||
| return 0 | |||
| ;; | |||
| 17) | |||
There was a problem hiding this comment.
Probably should drop support for 15 and 16, especially if 17-related changes were made (e.g. dropping constant expression stuff).
| "${TEST_RUNNER_INCLUDE_DIR}/test_runner/TestRunner.h" | ||
| "${TEST_RUNNER_INCLUDE_DIR}/test_runner/TestOutputSpec.h" | ||
| ) | ||
| #add_library( |
No description provided.