> On 8/17/23 9:23 AM, Yonghong Song wrote:
>> On 8/17/23 1:01 AM, Jose E. Marchesi wrote:
>>>
>>>> [...]
>>>> In llvm, for inline asm, 0xfffffffe, 4294967294 and -2 have the same
>>>> 4-byte bit-wise encoding, so they will be all encoded the same
>>>> 0xfffffffe in the actual insn.
>>>>
>>>> The following is an example for x86 target in llvm:
>>>>
>>>> $ cat t.c
>>>> int foo() {
>>>> int a, b;
>>>>
>>>> asm volatile("movl $0xfffffffe, %0" : "=r"(a) :);
>>>> asm volatile("movl $-2, %0" : "=r"(b) :);
>>>> return a + b;
>>>> }
>>>> $ clang -O2 -c t.c
>>>> $ llvm-objdump -d t.o
>>>>
>>>> t.o: file format elf64-x86-64
>>>>
>>>> Disassembly of section .text:
>>>>
>>>> 0000000000000000 <foo>:
>>>> 0: b9 fe ff ff ff movl $0xfffffffe, %ecx #
>>>> imm = 0xFFFFFFFE
>>>> 5: b8 fe ff ff ff movl $0xfffffffe, %eax #
>>>> imm = 0xFFFFFFFE
>>>> a: 01 c8 addl %ecx, %eax
>>>> c: c3 retq
>>>> $
>>>>
>>>> Whether it is 0xfffffffe or -2, the insn encoding is the same
>>>> and disasm prints out 0xfffffffe.
>>>
>>> Thanks for the explanation.
>>>
>>> I have pushed the commit below to binutils that makes GAS match the llvm
>>> assembler behavior regarding constant immediates. With this patch there
>>> are no more assembler errors when building the kernel bpf selftests.
>> Great! Thanks.
>>
>>>
>>> Note however that there is one pending divergence in the behavior of
>>> both assemblers when facing invalid programs where immediate operands
>>> cannot be represented in the number of bits of the field like in:
>>>
>>> $ cat foo.s
>>> if r1 > r2 goto 0x3fff1
>>>
>>> llvm silently truncates it to 16-bit:
>>>
>>> $ clang -target bpf foo.s
>>> $ bpf-unkonwn-none-objdump -M pseudoc -dr foo.o
>>> 0000000000000000 <.text>:
>>> 0: 2d 21 f1 ff 00 00 00 00 if r1>r2 goto -15
>>>
>>> GAS emits an error instead:
>>>
>>> $ as -mdialect=pseudoc foo.s
>>> foo.s: Assembler messages:
>>> foo.s:1: Error: pc-relative offset out of range, shall fit in 16 bits.
>>>
>>> (The same happens with 32-bit immediates.)
>>>
>>> We think the error is pertinent, and we recommend the llvm assembler to
>>> behave the same way.
>> Thanks! We will take a look at this issue soon.
>
> A patch like below can issue the warning for the above case:
>
> diff --git a/llvm/lib/Target/BPF/MCTargetDesc/BPFMCCodeEmitter.cpp
> b/llvm/lib/Target/BPF/MCTargetDesc/BPFMCCodeEmitter.cpp
> index 420a2aad480a..fca6bf30fb4b 100644
> --- a/llvm/lib/Target/BPF/MCTargetDesc/BPFMCCodeEmitter.cpp
> +++ b/llvm/lib/Target/BPF/MCTargetDesc/BPFMCCodeEmitter.cpp
> @@ -136,6 +136,12 @@ void BPFMCCodeEmitter::encodeInstruction(const
> MCInst &MI,
> OSE.write<uint16_t>(0);
> OSE.write<uint32_t>(Imm >> 32);
> } else {
> + if (Opcode == BPF::JUGT_rr) {
> + const MCOperand &MO = MI.getOperand(2);
> + int64_t Imm = MO.isImm() ? MO.getImm() : 0;
> + if (Imm > INT16_MAX || Imm < INT16_MIN)
Shouldn't that be:
if (Imm > UINT16_MAX || Imm < INT16_MIN)
?
> + report_fatal_error("Branch target out of insn range");
> + }
> // Get instruction encoding and emit it
> uint64_t Value = getBinaryCodeForInstr(MI, Fixups, STI);
> CB.push_back(Value >> 56);
>
> Need to generalize to other related conditional/unconditional
> operands. Will have a formal patch for llvm soon.
>
> Thanks.
