Hi Björn, Thanks for the comments! Inlined responses below: On Mon, Mar 2, 2020 at 11:50 PM Björn Töpel <bjorn.topel@xxxxxxxxx> wrote: > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Common functionality for RV32 and RV64 BPF JIT compilers > > + * > > + * Copyright (c) 2019 Björn Töpel <bjorn.topel@xxxxxxxxx> > > + * Copyright (c) 2020 Luke Nelson <luke.r.nels@xxxxxxxxx> > > + * Copyright (c) 2020 Xi Wang <xi.wang@xxxxxxxxx> > > I'm no lawyer, so this is more of a question; You've pulled out code > into a header, and renamed two functions. Does that warrant copyright > line additions? Should my line be removed? This header also includes new code for emitting instructions required for the RV32 JIT (e.g., sltu) and some additional pseudoinstructions (e.g., bgtu and similar). I'm also no lawyer, so I don't know either if this rises to the level of adding copyright lines. I'm happy to do the following in v5 if it looks better: + * Copyright (c) 2019 Björn Töpel <bjorn.topel@xxxxxxxxx> + * + * Modified by ... > > +#if __riscv_xlen == 64 > > Please remove this. If the inlined functions are not used, they're not > part of the binary. This adds complexity to the code, and without it > we can catch build errors early on! I agree in general we should avoid #if. The reason for using it here is to cause build errors if the RV32 JIT ever tries to emit an RV64-only instruction by mistake. Otherwise, what is now a build error would be delayed to an illegal instruction trap when the JITed code is executed, which is much harder to find and diagnose. We could use separate files, bpf_jit_32.h and bpf_jit_64.h (the latter will include the former), if we want to avoid #if. Though this adds another form of complexity. So the options here are 1) using no #if, with the risk of hiding subtle bugs in the RV32 JIT; 2) using #if as is; and 3) using separate headers. What do you think? Thanks! Luke
