Re: [PATCH V4 2/3] sim: eBPF simulator

["Jose E. Marchesi" <jose.marchesi@oracle.com>]

Hi Andrew.
    
    > This patch introduces the basics of an instruction-simulator for eBPF.
    > The simulator is based on CGEN.
    
    Thanks I took a look through and I'm basically happy.  There's a few
    small issues that I've highlighted inline below.

Thanks for the review.
    
    > 
    > sim/ChangeLog:
    > 
    > 2020-07-10  Jose E. Marchesi  <jose.marchesi@oracle.com>
    > 	    David Faust <david.faust@oracle.com>
    > 
    > 	* configure.tgt (sim_arch): Add entry for bpf-*-*.
    > 	* configure: Regenerate.
    > 	* MAINTAINERS: Add maintainer for the BPF simulator.
    > 	* bpf/Makefile.in: New file.
    > 	* bpf/bpf-helpers.c: Likewise.
    > 	* bpf/bpf-helpers.def: Likewise.
    > 	* bpf/bpf-helpers.h: Likewise.
    > 	* bpf/bpf-sim.h: Likewise.
    > 	* bpf/bpf.c: Likewise.
    > 	* bpf/config.in: Likewise.
    > 	* bpf/configure.ac: Likewise.
    > 	* bpf/decode.h: Likewise.
    > 	* bpf/eng.h: Likewise.
    > 	* bpf/mloop.in: Likewise.
    > 	* bpf/sim-if.c: Likewise.
    > 	* bpf/sim-main.h: Likewise.
    > 	* bpf/traps.c: Likewise.
    > 	* bpf/configure: Generate.
    
    This file is missing from this message.

Yeah, I removed the thunks containing complete `configure' files to
avoid hitting the mailman zise limit.  I can send it in a separated
email if required.

    > diff --git a/sim/bpf/Makefile.in b/sim/bpf/Makefile.in
    > new file mode 100644
    > index 0000000000..b12bb6a7d5
    > --- /dev/null
    > +++ b/sim/bpf/Makefile.in
    > @@ -0,0 +1,205 @@
    > +# Makefile template for configure for the eBPF simulator
    > +# Copyright (C) 2019 Free Software Foundation, Inc.
    
    This should probably be 2020 here.

Right.  Will change.

    > +# Dependencies for binaries from CGEN generated source
    > +
    > +arch.o: arch.c $(SIM_MAIN_DEPS)
    > +cpu.o: cpu.c $(BPF_INCLUDE_DEPS)
    > +decode-le.o: decode-le.c $(BPF_INCLUDE_DEPS)
    > +decode-be.o: decode-be.c $(BPF_INCLUDE_DEPS)
    > +# XXX sem-switch.o: sem-switch.c $(BPF_INCLUDE_DEPS)
    > +# XXX model.o: model.c $(BPF_INCLUDE_DEPS)
    
    As I mentioned in my review of patch #1, I'm not a fan of these XXX
    comments.  I'm not going to insist these be removed (where they are
    limited to the bpf directories), but I don't think things like the
    above add any value, so I'd prefer they be removed, and you shouldn't
    assume that these wont be removed in the future.

Actually, these particular comments are obsolete and should be removed.

    > diff --git a/sim/bpf/bpf-helpers.def b/sim/bpf/bpf-helpers.def
    > new file mode 100644
    > index 0000000000..6106ac794a
    > --- /dev/null
    > +++ b/sim/bpf/bpf-helpers.def
    > @@ -0,0 +1,194 @@
    > +/* BPF helpers database.
    > +   Copyright (C) 2019-2020 Free Software Foundation, Inc.
    
    Is this really 2019-2020?  If it's copied from some other file that is
    2019-2020 then could you mention on the preceding line, just to make
    it clear.

Yes, that file (with same name) was contributed as part of the BPF GCC
backend, back in 2019.