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

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

>> This patch introduces the basics of an instruction-simulator for eBPF.
>> The simulator is based on CGEN.
>> 
>> sim/ChangeLog:
>> 
>> 2020-08-03  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.
>
> You need to regenerate bpf/configure and commit it.

I removed that thunk from the patch due to the size limit.  It is in.

>> 	* 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.
>> 	* bpf/aclocal.m4: Likewise.
>> 
>> sim/testsuite/ChangeLog:
>> 
>> 2020-08-03  David Faust  <david.faust@oracle.com>
>> 	    Jose E. Marchesi  <jose.marchesi@oracle.com>
>> 
>> 	* configure: Regenerate.
>> 	* sim/bpf/allinsn.exp: New file.
>> 	* sim/bpf/alu.s: Likewise.
>> 	* sim/bpf/alu32.s: Likewise.
>> 	* sim/bpf/endbe.s: Likewise.
>> 	* sim/bpf/endle.s: Likewise.
>> 	* sim/bpf/jmp.s: Likewise.
>> 	* sim/bpf/jmp32.s: Likewise.
>> 	* sim/bpf/ldabs.s: Likewise.
>> 	* sim/bpf/mem.s: Likewise.
>> 	* sim/bpf/mov.s: Likewise.
>> 	* sim/bpf/testutils.inc: Likewise.
>> 	* sim/bpf/xadd.s: Likewise.
>> ---
>>  gdb/ChangeLog                       |     4 +
>>  gdb/configure.tgt                   |     1 +
>
> Make sure you include the gdb/ChangeLog entry in the commit message.

Ok.

>> +# Note the following files are generated in objdir, not srcdir.
>> +
>> +stamp-mloop: stamp-mloop-le stamp-mloop-be
>> +
>> +stamp-mloop-le: $(srcdir)/../common/genmloop.sh mloop.in Makefile
>> +	$(SHELL) $(srccom)/genmloop.sh -shell $(SHELL) \
>> +		-mono -scache -prefix bpfbf_ebpfle -cpu bpfbf \
>> +                -infile $(srcdir)/mloop.in
>> +	$(SHELL) $(srcroot)/move-if-change eng.hin eng-le.h
>> +	$(SHELL) $(srcroot)/move-if-change mloop.cin mloop-le.c
>> +	touch $@
>> +mloop-le.c eng-le.h: stamp-mloop-le
>> +	@true
>> +
>> +stamp-mloop-be: $(srcdir)/../common/genmloop.sh mloop.in Makefile
>> +	$(SHELL) $(srccom)/genmloop.sh -shell $(SHELL) \
>> +		-mono -scache -prefix bpfbf_ebpfbe -cpu bpfbf \
>> +                -infile $(srcdir)/mloop.in
>> +	$(SHELL) $(srcroot)/move-if-change eng.hin eng-be.h
>> +	$(SHELL) $(srcroot)/move-if-change mloop.cin mloop-be.c
>> +	touch $@
>> +mloop-be.c eng-be.h: stamp-mloop-be
>> +	@true
>
> When testing the build I ran into issues here while trying to perform
> a parallel build as both these rules generate the files eng.hin and
> mloop.cin.
>
> I know this problem is not unique to this target, but it would be nice
> if we didn't introduce more places where the parallel build is broken.
>
> The _correct_ solution would be to update genmloop.sh so that the
> output files can be named.  However, I'm aware that this is an
> unreasonable expectation, so could you at least combine the be and le
> generation into a single rule 'stamp-all-mloop' please, and add a
> comment explaining why.  This at least will work around the problem
> for now.

Regarding genmloop.sh, I just noticed the script may be already
supporting this?

	-outfile-suffix) shift ; outsuffix=$1 ;;

I'm using it now like this, and seems to work well with -j:

# Note the following files are generated in objdir, not srcdir.

stamp-mloop: stamp-mloop-le stamp-mloop-be

stamp-mloop-le: $(srcdir)/../common/genmloop.sh mloop.in Makefile
	$(SHELL) $(srccom)/genmloop.sh -shell $(SHELL) \
		-mono -scache -prefix bpfbf_ebpfle -cpu bpfbf \
                -infile $(srcdir)/mloop.in -outfile-suffix -le
	$(SHELL) $(srcroot)/move-if-change eng-le.hin eng-le.h
	$(SHELL) $(srcroot)/move-if-change mloop-le.cin mloop-le.c
	touch $@
mloop-le.c eng-le.h: stamp-mloop-le
	@true

stamp-mloop-be: $(srcdir)/../common/genmloop.sh mloop.in Makefile
	$(SHELL) $(srccom)/genmloop.sh -shell $(SHELL) \
		-mono -scache -prefix bpfbf_ebpfbe -cpu bpfbf \
                -infile $(srcdir)/mloop.in -outfile-suffix -be
	$(SHELL) $(srcroot)/move-if-change eng-be.hin eng-be.h
	$(SHELL) $(srcroot)/move-if-change mloop-be.cin mloop-be.c
	touch $@
mloop-be.c eng-be.h: stamp-mloop-be
	@true

> With these issues resolved I'm happy for this to go in.

Thanks!