[PATCH] D124413: [llvm-ml] Improve indirect call parsing
Alan Zhao via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Tue Apr 26 17:10:39 PDT 2022
ayzhao added inline comments.
================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:2642
+ if (Parser.isParsingMasm()) {
+ bool IsUnconditionalBranch =
+ Name.equals_insensitive("jmp") || Name.equals_insensitive("call");
----------------
epastor wrote:
> Why do we need to set the DefaultBaseReg unconditionally for the unconditional branches? Aren't they only implicitly RIP-relative if referencing a named symbol (i.e., one with known type, which is what the SM.getElementSize() below is testing)?
>
> (Also, I'm a little surprised if the conditional branches have different handling, though anything's possible.)
> Why do we need to set the DefaultBaseReg unconditionally for the unconditional branches? Aren't they only implicitly RIP-relative if referencing a named symbol (i.e., one with known type, which is what the SM.getElementSize() below is testing)?
Done.
The difficulty here is that in direct jumps, the operand is a relative displacement, not a memory address. My original thought was that direct jumps are inherently `rip` relative (e.g. `jmp 0xAB` means jump to the address `[rip + 0xAB]`, not jump to `0xAB`], and direct jumps and indirect jumps (e.g. jump to the address `[rip + 0xAB]` vs jump to the address stored at memory location `[rip + 0xAB]` would be differentiated by `BaseReg` ad `MaybeDirectBranchDest`. In hindsight though, this would've meant that we have to set `DefaultBaseReg = X86::RIP` for all branching instructions in 64-bit mode, which feels pretty excessive.
To complicate things, there are also far jumps, which apparently do jump to absolute addresses. This patch ignores those.
> (Also, I'm a little surprised if the conditional branches have different handling, though anything's possible.)
All branching instructions that I've encountered only accept offsets as parameters[0][1], so I _think_ only `call` and `jmp` can do an indirect branch. OTOH, I could be completely missing something.
[0]: https://www.felixcloutier.com/x86/jcc
[1]: https://www.felixcloutier.com/x86/loop:loopcc
================
Comment at: llvm/lib/Target/X86/AsmParser/X86AsmParser.cpp:2658
+ if (is64BitMode()) {
+ if (TypeName.equals_insensitive("QWORD"))
+ MaybeDirectBranchDest = false;
----------------
epastor wrote:
> Can't we check the type size, rather than the name? If it's actually special-cased on these types alone, then maybe we should consider marking the type info with a special flag rather than handling the mnemonic here.
I played around with MSVC ml and it turns out that it is entirely dependent on the type size rather than the name, so, for example, an 8 byte sized struct would still work. I added a test case for this scenario.
================
Comment at: llvm/test/tools/llvm-ml/unconditional_branch_x64.asm:5
+
+fn_ref QWORD 28
+fn PROC
----------------
hans wrote:
> probably a silly question, but what's the 28 for?
Dummy value
================
Comment at: llvm/test/tools/llvm-ml/unconditional_branch_x64.asm:70
+; CHECK: jmp qword ptr [rip + fn]
\ No newline at end of file
----------------
hans wrote:
> These are great test cases!
>
> Can we also have some tests where instead of jmp or call, "fn", "[fn]" etc. are moved into a register?
> And should there be some tests with conditional jumps too?
> It seems from this patch that they might be different, but in either case it seems good to cover that with tests.
> Can we also have some tests where instead of jmp or call, "fn", "[fn]" etc. are moved into a register?
Already covered in [rip-relative-addressing.asm](https://github.com/llvm/llvm-project/blob/main/llvm/test/tools/llvm-ml/rip-relative-addressing.asm)
> And should there be some tests with conditional jumps too?
Done (though conditional jumps only accept labels)
================
Comment at: llvm/test/tools/llvm-ml/unconditional_branch_x86.asm:1
+; RUN: llvm-ml -m32 -filetype=s %s /Fo - | FileCheck %s
+
----------------
hans wrote:
> Since they're mostly the same, maybe instead of having separate test files for 32- and 64-bit, maybe it would be simpler to combine them into one. file with two "RUN" lines.
> You could use a macro to define e.g. "WORD" to "qword" or "dword", and use "CHECK32" and "CHECK64" for when the expectations are different.
Done.
At this time, type aliases are defined with the `TYPEDEF` keyword, which is not yet implemented, so I ended up splitting it into multiple blocks.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D124413/new/
https://reviews.llvm.org/D124413
More information about the llvm-commits
mailing list