Add:
- RV64C instructions sets.
- corresponding unittests.
- c.break code for lldb and lldb-server
Fix:
- wrong decoding of imm in DecodeSType
Details
Details
Diff Detail
Diff Detail
Event Timeline
Comment Actions
Cool to see the lowering in action. A few comments but (arch details aside) this is looking good.
| lldb/source/Plugins/Instruction/RISCV/RISCVCInstructions.h | ||
|---|---|---|
| 143 | No bit 0 because you'll never be branching to an odd address, correct? | |
| 146 | Obvious question is obvious - but, why are we decoding a compressed instruction but being given 32 bits? | |
| lldb/source/Target/Platform.cpp | ||
| 1933 | Not a problem for this change but I'm curious. Do you know how this flag is set? Perhaps when we know that the original instruction is 2 bytes this will be set. For arm/thumb we have this eCodeAlternateISA that handles it. | |
| lldb/unittests/Instruction/RISCV/TestRISCVEmulator.cpp | ||
| 267 | Why is the visit required? Why not just memcmp lhs and rhs? | |
Comment Actions
Oh and is this a specific named subset of RVC? Would be good to be specific in the commit title if there is a name for this set you are adding.
| lldb/source/Plugins/Instruction/RISCV/RISCVCInstructions.h | ||
|---|---|---|
| 143 | Yes, they are aligned to 2-byte according to the spec.
| |
| 146 | I used a decode jump table which requires a decode function of type uint32_t -> RISCVInst, so the type here was just to make compiler happy.
| |
| lldb/source/Target/Platform.cpp | ||
| 1933 | The flag was set according to the EF_RISCV_RVC bit in the e_flags in the elf header (see the code in ObjectFileELF). The RISCV elf spec says
| |
| lldb/unittests/Instruction/RISCV/TestRISCVEmulator.cpp | ||
| 267 | I'm unsure if std::variant can be compared byte-wise as we may work with different STL implementations. | |
Comment Actions
You could add static asserts with https://en.cppreference.com/w/cpp/types/is_trivial if you wanted to enforce the POD-ness of the instructions. Up to you though. I don't see a need for complex fields and it would probably fail quite fast over a few test runs in any case.
This LGTM with the one comment ammended.
| lldb/source/Target/Platform.cpp | ||
|---|---|---|
| 1933 | Cool, so it will "just work" already then. That's what I wondered. | |
| lldb/unittests/Instruction/RISCV/TestRISCVEmulator.cpp | ||
| 267 | Good point, let's not do that. And now I see that the .index() check is making sure that the two enclosed types are the same. So it doesn't matter that some of the instructions have the same memory layout, they won't be memcmp anyway. | |
| 272 | Please add on the end of this (they are the same instruction type) just for folks like me who aren't so familiar with variant. | |
Comment Actions
After commit 05ae747a5353811f93f5814f24d2335e6229d78a ("[LLDB][RISCV] Add RV64C instruction support for EmulateInstructionRISCV"),
build failed when "ninja check-lldb" on my local machine:
[1/3] Building CXX object tools/lldb/unittests/Instruction/CMakeFiles/EmulatorTests.dir/RISCV/TestRISCVEmulator.cpp.o FAILED: tools/lldb/unittests/Instruction/CMakeFiles/EmulatorTests.dir/RISCV/TestRISCVEmulator.cpp.o /usr/bin/c++ -DGTEST_HAS_RTTI=0 -DHAVE_ROUND -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/home/loongson/llvm-project/llvm/build/tools/lldb/unittests/Instruction -I/home/loongson/llvm-project/lldb/unittests/Instruction -I/home/loongson/llvm-project/lldb/include -I/home/loongson/llvm-project/llvm/build/tools/lldb/include -I/home/loongson/llvm-project/llvm/build/include -I/home/loongson/llvm-project/llvm/include -I/home/loongson/llvm-project/llvm/../clang/include -I/home/loongson/llvm-project/llvm/build/tools/lldb/../clang/include -I/home/loongson/llvm-project/lldb/source -I/home/loongson/llvm-project/lldb/unittests -I/home/loongson/llvm-project/llvm/utils/unittest/googletest/include -I/home/loongson/llvm-project/llvm/utils/unittest/googlemock/include -isystem /usr/include/libxml2 -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wno-misleading-indentation -Wctad-maybe-unsupported -fdiagnostics-color -ffunction-sections -fdata-sections -Wno-deprecated-declarations -Wno-unknown-pragmas -Wno-strict-aliasing -Wno-stringop-truncation -O3 -DNDEBUG -include /home/loongson/llvm-project/lldb/unittests/gtest_common.h -Wno-variadic-macros -fno-exceptions -fno-rtti -Wno-suggest-override -std=c++17 -MD -MT tools/lldb/unittests/Instruction/CMakeFiles/EmulatorTests.dir/RISCV/TestRISCVEmulator.cpp.o -MF tools/lldb/unittests/Instruction/CMakeFiles/EmulatorTests.dir/RISCV/TestRISCVEmulator.cpp.o.d -o tools/lldb/unittests/Instruction/CMakeFiles/EmulatorTests.dir/RISCV/TestRISCVEmulator.cpp.o -c /home/loongson/llvm-project/lldb/unittests/Instruction/RISCV/TestRISCVEmulator.cpp c++: fatal error: Killed signal terminated program cc1plus compilation terminated. ninja: build stopped: subcommand failed.
After revert the changes in lldb/unittests/Instruction/RISCV/TestRISCVEmulator.cpp, build successful.
Comment Actions
I failed to repro this error. :(
c++: fatal error: Killed signal terminated program cc1plus
With such a vague error message it is hard to find the problematic part and it does not look like a code problem. Would you mind checking your build configuration and trying ninja check-lldb -j1 for detailed information?
Comment Actions
dmesg shows that the compiler is getting killed by the OOM-killer due to using too many resources.
Adding 64G swap, it is still OOM. Ref: https://github.com/soedinglab/hh-suite/issues/280
Using clang++ to build TestRISCVEmulator.cpp is OK, so maybe this issue is related with gcc.
Comment Actions
Can you give us:
- the full cmake command you used to configure the project
- the gcc/g++ version used
- the distro used
- the architecture (unlikely to matter but I'm assuming Loongson?)
- the version of clang++ that did work
Could be tripping over a bug in g++. If that version is >= the minimum llvm requires we should look for a workaround.
One thing you could try is removing parts of TestRISCVEmulator.cpp and seeing what breaks the build. Though this is best done by @Emmmer, if it can be reproduced elsewhere.
Comment Actions
git clone https://github.com/llvm/llvm-project.git
mkdir -p llvm-project/llvm/build
cd llvm-project/llvm/build
cmake .. -G "Ninja" -DLLVM_TARGETS_TO_BUILD="BPF" \
-DLLVM_EXPERIMENTAL_TARGETS_TO_BUILD="LoongArch" \
-DLLVM_ENABLE_PROJECTS="clang;lldb" \
-DCMAKE_BUILD_TYPE=Release \
-DLLVM_BUILD_RUNTIME=OFF
ninja
ninja check-lldb -j1
- the gcc/g++ version used
gcc (GCC) 13.0.0 20220919
g++ (GCC) 13.0.0 20220919
- the distro used
clfs loongarch
- the architecture (unlikely to matter but I'm assuming Loongson?)
LoongArch
- the version of clang++ that did work
clang version 14.0.6
Could be tripping over a bug in g++. If that version is >= the minimum llvm requires we should look for a workaround.
One thing you could try is removing parts of TestRISCVEmulator.cpp and seeing what breaks the build. Though this is best done by @Emmmer, if it can be reproduced elsewhere.
This is introduced in the following commit:
$ git log -p lldb/unittests/Instruction/RISCV/TestRISCVEmulator.cpp commit 05ae747a5353811f93f5814f24d2335e6229d78a Author: Emmmer <yjhdandan@163.com> Date: Thu Oct 20 23:05:37 2022 +0800 [LLDB][RISCV] Add RV64C instruction support for EmulateInstructionRISCV
c++ OOM is related with this vector, if it is empty, building is OK, if it has one element at least, build failed.
std::vector<TestDecode> tests = {
};@SixWeining can also reproduce it on his local machine.
Comment Actions
I suspect the cause is that g++ tries to instantiate these generic lambdas inside std::visit, which consumes N^2 the size memory (or whatever is related?) when there's nested-std::visit
bool compareInst(const RISCVInst &lhs, const RISCVInst &rhs) {
if (lhs.index() != rhs.index())
return false;
return std::visit(
[&](auto &&L) {
return std::visit(
[&](auto &&R) {
return std::memcmp(&L, &R, sizeof(L)) == 0;
},
rhs);
},
lhs);
}we can try to change this code to :
bool compareInst(const RISCVInst &L, const RISCVInst &R) {
return std::visit(
[](auto &&lhs, auto &&rhs) {
return std::is_same_v<decltype(lhs), decltype(rhs)> &&
memcmp(&lhs, &rhs, sizeof(lhs)) == 0;
},
L, R);
}But I’m unsure if it will work.
Comment Actions
I can reproduce this issue on x86_64 used with the latest gcc.
(1) Install Fedora36
https://download.fedoraproject.org/pub/fedora/linux/releases/36/Workstation/x86_64/iso/Fedora-Workstation-Live-x86_64-36-1.5.iso
$ uname -a
Linux fedora 5.17.5-300.fc36.x86_64 #1 SMP PREEMPT Thu Apr 28 15:51:30 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux
(2) Install some packages
sudo yum -y install llvm clang g++ cmake ninja-build bison flex texinfo
$ gcc --version
gcc (GCC) 12.2.1 20220819 (Red Hat 12.2.1-2)
$ c++ --version
c++ (GCC) 12.2.1 20220819 (Red Hat 12.2.1-2)
$ clang --version
clang version 14.0.5 (Fedora 14.0.5-1.fc36)
$ clang++ --version
clang version 14.0.5 (Fedora 14.0.5-1.fc36)
(3) Build llvm
git clone https://github.com/llvm/llvm-project.git
mkdir -p llvm-project/llvm/build
cd llvm-project/llvm/build
cmake .. -G "Ninja" -DLLVM_TARGETS_TO_BUILD="X86" \
-DLLVM_ENABLE_PROJECTS="clang;lldb" \
-DCMAKE_BUILD_TYPE=Release \
-DLLVM_BUILD_RUNTIME=OFF
ninja
ninja check-lldbThis is OK used with gcc/c++ 12.
(4) Update gcc to the latest version
git clone git://gcc.gnu.org/git/gcc.git
cd gcc
./contrib/download_prerequisites
mkdir -p build
cd build
../configure --prefix=/usr/local/gcc --enable-checking=release --enable-languages=c,c++ --disable-multilib
make
sudo make install
export PATH=/usr/local/gcc/bin/:$PATH
export LD_LIBRARY_PATH=/usr/local/gcc/lib64:$LD_LIBRARY_PATH
$ gcc --version
gcc (GCC) 13.0.0 20221029 (experimental)
$ c++ --version
c++ (GCC) 13.0.0 20221029 (experimental)
(5) Do the above (3) again used with gcc/c++ 13
git clone https://github.com/llvm/llvm-project.git
mkdir -p llvm-project/llvm/build
cd llvm-project/llvm/build
cmake .. -G "Ninja" -DLLVM_TARGETS_TO_BUILD="X86" \
-DLLVM_ENABLE_PROJECTS="clang;lldb" \
-DCMAKE_BUILD_TYPE=Release \
-DLLVM_BUILD_RUNTIME=OFF \
-DCMAKE_C_COMPILER="/usr/local/gcc/bin/gcc" \
-DCMAKE_CXX_COMPILER="/usr/local/gcc/bin/c++"
ninja
ninja check-lldbWe can see that "ninja check-lldb" failed due to OOM
used with gcc/c++ 13.
Comment Actions
Then I guess it is a gcc-specific bug and you could try to report this to their developers.
git clone https://github.com/llvm/llvm-project.git mkdir -p llvm-project/llvm/build cd llvm-project/llvm/build cmake .. -G "Ninja" -DLLVM_TARGETS_TO_BUILD="X86" \ -DLLVM_ENABLE_PROJECTS="clang;lldb" \ -DCMAKE_BUILD_TYPE=Release \ -DLLVM_BUILD_RUNTIME=OFF \ -DCMAKE_C_COMPILER="/usr/local/gcc/bin/gcc" \ -DCMAKE_CXX_COMPILER="/usr/local/gcc/bin/c++" ninja ninja check-lldb
We can try arc patch --diff 137041 before cmake and see if it failed.
Let's move to D137041
No bit 0 because you'll never be branching to an odd address, correct?