-
Notifications
You must be signed in to change notification settings - Fork 14.5k
[llvm-exegesis] [AArch64] Resolving "snippet crashed while running: Segmentation fault" for Load Instructions #142552
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Switched X16 as temporary register in loadFPCRImmediate instead of X8 which is used by syscalls
…re syscall registers and syscall generator
…ions used by subprocess execution mode.
…ters requiring memory address.
@llvm/pr-subscribers-tools-llvm-exegesis Author: Lakshay Kumar (lakshayk-nv) ChangesWe want to support load and store instructions for aarch64 but currently they throw segmentation fault as register are initialized by 0 but load instruction requires register to be loaded with valid memory address. A. Prerequisite for this is to support
|
Why exactly did you need to support |
✅ With the latest revision this PR passed the C/C++ code formatter. |
…project into llvm-exegesis-segfault
It is prerequisite only if loading registers with address to auxiliary mmap (Updated initial comment accordingly).
We want to get some feedback on scratchMemoryRegister route.
So, To enable load instructions in
Q0. Is updating Q1.1. [sanity check] Scratch memory register be an arbitrary register (i.e. X14) is correct ? Q2. |
This is a large change, which shows that there are a lot of moving part involved here. I understand and agree it is not easy to see what is necessary to fully support loads/stores (for AArch64). Thanks for putting up this WIP patch, maybe we can use this WIP patch to define the minimum change and use-case that we want to add first. I don't think we need to strive for complete support. So, again, I would like to see a minimal change that improves some opcodes, doesn't regress others, and very clearing stating the assumptions and things that can or should be picked up in follow ups. Correct me if I am wrong, or if you have other opinions, but this is the first scenario I would like to see supported:
If we can get some new opcodes working with this strategy, and if they give some sensible results, then I think that is forward progress. If you agree with this exercise, then let's go ahead with this, first thing you can do is to strip-out everything that is not needed to support this minimal change. I don't mind either way, if you implement this minimal change in this merge request here, or keep this one around as a reference and open a new one for the minimal change. @davemgreen or @boomanaiden154:, if you have opinions on any of this, please let us know. |
Could be. Not sure why exactly this would be different between X86 and AArch64 though. Probably something that should be understood before landing the patch, but doesn't hurt to try.
No. The scratch memory register is defined by the calling convention. It's the first argument of the call that jumps to the assembly snippet.
That comes up usually because a physical register is undefined at the MC verification stage. You'll probably need to define the register somewhere.
The scratch memory register support should already work on AArch64 and thus would be a lot simpler. For using |
I had checked and not found any regression on x86( AArch64 doesn't have any instructions
Sure, Will update implementation to get Moreover, we can put in some effort to resolve illegal instruction when using This was due to the question |
Added changes includes implementation of required functions (mmap, munmap and configurePerfCounter), saving auxiliary memory address to stack and temporary fix to load first register via mmap address and rest by setRegto as done previously, which resolve most AArch64 load instructions. Moreover, I wanted to understand x86 implementation for Memory Annotation (from @boomanaiden154, as you owner/implementor of them) Q2. How is File Descriptor being managed ?
But file descriptor at this or any address seems not to populated i.e. No call for Q2.2 Similarly for Auxiliary Memory requires Q3. File Descriptor for configurePerfCounter? |
We don't support putting an address directly into a register in subprocess mode currently. For now you have to specify a specific address and then add add an instruction/snippet setup to set a register to that value. This has been something I've wanted to add for a while, but I've never had a need for it.
Probably depends upon the specific call. Most likely just to clean things up at the end. It's probably not strictly necessary in a lot of cases.
Which file descriptor? I believe there is an FD for the shared memory that is created before we fork the subprocess and thus just exists in the child. The perf counter FD needs to be created afterwards to target the PID of the subprocess. We send it to the child process through a socket to perform FD translation. There was original implementation using
It's done in the parent process and sent over. Most of the subprocess memory stuff should be handled by
Not initialize as in you have a debugger attached and the value is zero/random or not initialize as in you can't see where it's getting initialized? The tests pass, so it's definitely getting initialized at least in some contexts. |
We want to support load and store instructions for aarch64 but currently they throw segmentation fault as register are initialized by 0 but load instruction requires register to be loaded with valid memory address.
This is a WIP patch and not expecting to merging it in current state but to get feedback.
Load registers requiring memory address
There are possibly two ways to support load instructions (setting registers with valid memory address):-
1. With address to auxiliary mmap:
Prerequisite for this is to support
--mode=subprocess
for AArch64.Adding support for
--mode=subprocess
and Memory Annotation of manual snippet.configurePerfCounter()
.Generating syscall for aux mmap, save its return value in stack and load required registers with memory address.
Implemented the same, currently.
TODO: how to differentiate between register requiring address and otherwise ?
For example: LD1B opcode (
ld1b {z6.b}, p4/z, [x14, x2]
) expects first register to contain address and second having offset value.Temporary fix: Init first register queried by instruction to loaded by address and rest by setRegTo as done previously.
2. Utilize
fillMemoryOperands
Found
fillMemoryOperands()
used by x86 implementation, seems relevant to init registers required by load instructions.Implementation for
fillMemoryOperands()
,getScratchMemoryRegister()
is missing for AArch64.Firstly, Codeflow check for
IsMemory
(i.e.OPERAND_MEMORY
) which is not relevant for AArch64.Thus, Experimentally added
IsMemory
to ORmayLoadOrStore
too (MCInstrDescView.cpp
)TODO: Implement
getScratchMemoryRegister()
correctlyreturn MCRegister()
result in register to not be valid and exit with"Infeasible : target does not support memory instructions"
TODO: Implement
fillMemoryOperands
PS: Added changes still required by this WIP Patch
Pathway 1: Enable load instructions in --mode=inprocess
Pathway 2: Enable load instructions in --mode=subprocess
fd=-1
fd=-1
Please review: @sjoerdmeijer, @boomanaiden154, @davemgreen
Looking forward for feedback.
Thanks,