On Thu, 2023-08-10 at 10:45 -0700, Yonghong Song wrote:
>
> On 8/10/23 10:39 AM, Jose E. Marchesi wrote:
> >
> > > On 8/10/23 3:35 AM, Jose E. Marchesi wrote:
> > > > Hello.
> > > > We found that some of the BPF selftests use the "p" constraint in
> > > > inline
> > > > assembly snippets, for input operands for MOV (rN = rM) instructions.
> > > > This is mainly done via the __imm_ptr macro defined in
> > > > tools/testing/selftests/bpf/progs/bpf_misc.h:
> > > > #define __imm_ptr(name) [name]"p"(&name)
> > > > Example:
> > > > int consume_first_item_only(void *ctx)
> > > > {
> > > > struct bpf_iter_num iter;
> > > > asm volatile (
> > > > /* create iterator */
> > > > "r1 = %[iter];"
> > > > [...]
> > > > :
> > > > : __imm_ptr(iter)
> > > > : CLOBBERS);
> > > > [...]
> > > > }
> > > > Little equivalent reproducer:
> > > > int bar ()
> > > > {
> > > > int jorl;
> > > > asm volatile ("r1 = %a[jorl]" : : [jorl]"p"(&jorl));
> > > > return jorl;
> > > > }
> > > > The "p" constraint is a tricky one. It is documented in the GCC
> > > > manual
> > > > section "Simple Constraints":
> > > > An operand that is a valid memory address is allowed. This is
> > > > for
> > > > ``load address'' and ``push address'' instructions.
> > > > p in the constraint must be accompanied by address_operand as the
> > > > predicate in the match_operand. This predicate interprets the mode
> > > > specified in the match_operand as the mode of the memory reference for
> > > > which the address would be valid.
> > > > There are two problems:
> > > > 1. It is questionable whether that constraint was ever intended to
> > > > be
> > > > used in inline assembly templates, because its behavior really
> > > > depends on compiler internals. A "memory address" is not the same
> > > > than a "memory operand" or a "memory reference" (constraint "m"), and
> > > > in fact its usage in the template above results in an error in both
> > > > x86_64-linux-gnu and bpf-unkonwn-none:
> > > > foo.c: In function ‘bar’:
> > > > foo.c:6:3: error: invalid 'asm': invalid expression as operand
> > > > 6 | asm volatile ("r1 = %[jorl]" : : [jorl]"p"(&jorl));
> > > > | ^~~
> > > > I would assume the same happens with aarch64, riscv, and
> > > > most/all
> > > > other targets in GCC, that do not accept operands of the form A + B
> > > > that are not wrapped either in a const or in a memory reference.
> > > > To avoid that error, the usage of the "p" constraint in internal
> > > > GCC
> > > > instruction templates is supposed to be complemented by the 'a'
> > > > modifier, like in:
> > > > asm volatile ("r1 = %a[jorl]" : : [jorl]"p"(&jorl));
> > > > Internally documented (in GCC's final.cc) as:
> > > > %aN means expect operand N to be a memory address
> > > > (not a memory reference!) and print a reference
> > > > to that address.
> > > > That works because when the modifier 'a' is found, GCC prints an
> > > > "operand address", which is not the same than an "operand".
> > > > But...
> > > > 2. Even if we used the internal 'a' modifier (we shouldn't) the 'rN
> > > > =
> > > > rM' instruction really requires a register argument. In cases
> > > > involving automatics, like in the examples above, we easily end with:
> > > > bar:
> > > > #APP
> > > > r1 = r10-4
> > > > #NO_APP
> > > > In other cases we could conceibly also end with a 64-bit label
> > > > that
> > > > may overflow the 32-bit immediate operand of `rN = imm32'
> > > > instructions:
> > > > r1 = foo
> > > > All of which is clearly wrong.
> > > > clang happens to do "the right thing" in the current usage of
> > > > __imm_ptr
> > > > in the BPF tests, because even with -O2 it seems to "reload" the
> > > > fp-relative address of the automatic to a register like in:
> > > > bar:
> > > > r1 = r10
> > > > r1 += -4
> > > > #APP
> > > > r1 = r1
> > > > #NO_APP
> > >
> > > Unfortunately, the modifier 'a' won't work for clang.
> > >
> > > $ cat t.c int bar () { int jorl; asm volatile ("r1 =
> > > %a[jorl]" : : [jorl]"p"(&jorl)); return jorl; } $ gcc -O2 -g -S
> > > t.c $ clang --target=bpf -O2 -g -S t.c clang:
> > > ../lib/Target/BPF/BPFAsmPrinter.cpp:126: virtual bool
> > > {anonymous}::BPFAsmPrinter::PrintAsmMemoryOperand(const
> > > llvm::MachineInstr*, unsigned int, const char*, llvm::raw_ostream&):
> > > Assertion `Offs
> > > etMO.isImm() && "Unexpected offset for inline asm memory operand."' failed.
> > > ...
> > >
> > > I guess BPF backend can try to add support for this 'a' modifier
> > > if necessary.
> >
> > I wouldn't advise that: it is an internal GCC detail that just happens
> > to work in inline asm. Also, even if you did that constraint may result
> > in operands that are not single registers. It would be better to use
> > "r" constraint instead.
>
> Sounds good. We also do not want to add support for this 'a' thing
> if there are alternatives.
>
> >
> > >
> > > > Which is what GCC would generate with -O0. Whether this is by chance or
> > > > by design (Nick, do you know?) I don't think the compiler should be
> > > > expected to do that reload driven by the "p" constraint.
> > > > I would suggest to change that macro (and similar out of macro
> > > > usages of
> > > > the "p" constraint in selftests/bpf/progs/iters.c) to use the "r"
> > > > constraint instead. If a register is what is required, we should let
> > > > the compiler know.
> > >
> > > Could you specify what is the syntax ("r" constraint) which will work
> > > for both clang and gcc?
> >
> > Instead of:
> >
> > #define __imm_ptr(name) [name]"p"(&name)
> >
> > Use this:
> >
> > #define __imm_ptr(name) [name]"r"(&name)
> >
> > That assures that the operand (the pointer value) will be available in
> > the form of a single register.
>
> Okay, this seems work for both gcc and clang.
> Eduard, what do you think about the above suggested change?
BPF selftests are passing with this change.
The macro in question is used in 3 files:
- verifier_subprog_precision.c
- iters_state_safety.c
- iters_looping.c
I don't see any difference in the generated object files
(at-least for cpuv4).
So, I guess we should be fine.
>
> >
> > >
> > > > Thoughts?
> > > > PS: I am aware that the x86 port of the kernel uses the "p"
> > > > constraint
> > > > in the percpu macros (arch/x86/include/asm/percpu.h) but that usage
> > > > is in a different context (I would assume it is used in x86
> > > > instructions that get constant addresses or global addresses loaded
> > > > in registers and not automatics) where it seems to work well.
> > > >
>
