diff mbox

[v2,06/13] fuzz/x86_emulate: Rename the file containing the wrapper code

Message ID 20170925142648.25959-6-george.dunlap@citrix.com (mailing list archive)
State New, archived
Headers show

Commit Message

George Dunlap Sept. 25, 2017, 2:26 p.m. UTC 
When generating coverage output, by default gcov generates output
filenames based only on the coverage file and the "leaf" source file,
not the full path.  As a result, it uses the same name for
x86_emulate.c and x86_emulate/x86_emulate.c, generally overwriting the
second (which we actually are about) with the first (which is just a
wrapper).

Rename the user-space wrapper helpers to x86_emulate_user.[ch], so
that it generates separate files.

There is actually an option to gcov, `--preserve-paths`, which will
cause the full path name to be included in the filename, properly
distinguishing between the two.  However, given that the user-space
wrapper doesn't actually do any emulation (and the poor state of gcov
documentation making it difficult to find the option in the first
place), it seems to make more sense to rename the file anyway.

Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---

NB: I discovered the `-p` option to gcov after writing this patch.
But I think the patch itself still makes sense.

CC: Ian Jackson <ian.jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
---
 tools/fuzz/x86_instruction_emulator/Makefile                 | 12 ++++++------
 tools/fuzz/x86_instruction_emulator/fuzz-emul.c              |  2 +-
 tools/tests/x86_emulator/Makefile                            |  6 +++---
 tools/tests/x86_emulator/test_x86_emulator.c                 |  2 +-
 .../tests/x86_emulator/{x86_emulate.c => x86_emulate_user.c} |  2 +-
 .../tests/x86_emulator/{x86_emulate.h => x86_emulate_user.h} |  0
 6 files changed, 12 insertions(+), 12 deletions(-)
 rename tools/tests/x86_emulator/{x86_emulate.c => x86_emulate_user.c} (99%)
 rename tools/tests/x86_emulator/{x86_emulate.h => x86_emulate_user.h} (100%)

Comments

Jan Beulich Oct. 4, 2017, 8:23 a.m. UTC | #1 
>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
> --- a/tools/fuzz/x86_instruction_emulator/Makefile
> +++ b/tools/fuzz/x86_instruction_emulator/Makefile
> @@ -18,22 +18,22 @@ asm:
>  
>  asm/%: asm ;
>  
> -x86_emulate.c x86_emulate.h: %:
> +x86_emulate_user.c x86_emulate_user.h: %:

How about avoiding the names getting even longer? E.g. using
x86-emulate.[ch] or x86emul-user.[ch] instead?

> @@ -42,7 +42,7 @@ all: x86-insn-fuzz-all
>  
>  .PHONY: distclean
>  distclean: clean
> -	rm -f x86_emulate x86_emulate.c x86_emulate.h asm
> +	rm -f x86_emulate x86_emulate_user.c x86_emulate_user.h asm

If you want to stick to the longer names, would you mind taking the
opportunity to make this just x86_emulate* ?

Jan
George Dunlap Oct. 4, 2017, 4:34 p.m. UTC | #2 
On 10/04/2017 09:23 AM, Jan Beulich wrote:
>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>> --- a/tools/fuzz/x86_instruction_emulator/Makefile
>> +++ b/tools/fuzz/x86_instruction_emulator/Makefile
>> @@ -18,22 +18,22 @@ asm:
>>  
>>  asm/%: asm ;
>>  
>> -x86_emulate.c x86_emulate.h: %:
>> +x86_emulate_user.c x86_emulate_user.h: %:
> 
> How about avoiding the names getting even longer? E.g. using
> x86-emulate.[ch] or x86emul-user.[ch] instead?

My original idea was to make it easy to construct the original filename
from the long filename.  I don't have super-strong opinions (mostly
because I think all the options I've seen are pretty bad), but I still
think that this is the least-bad option.

If you have strong opinions about one of the other ones, let me know and
I'll change it.

>> @@ -42,7 +42,7 @@ all: x86-insn-fuzz-all
>>  
>>  .PHONY: distclean
>>  distclean: clean
>> -	rm -f x86_emulate x86_emulate.c x86_emulate.h asm
>> +	rm -f x86_emulate x86_emulate_user.c x86_emulate_user.h asm
> 
> If you want to stick to the longer names, would you mind taking the
> opportunity to make this just x86_emulate* ?

What if you put something in that directly called
"x86_emulate_user.c.diff" (or something like that) and then ran "make
clean"?

I tend to think that 'make clean' should only clean things that it is
pretty confident were put there by the build system, and not the user.

 -George
Jan Beulich Oct. 5, 2017, 9:01 a.m. UTC | #3 
>>> On 04.10.17 at 18:34, <george.dunlap@citrix.com> wrote:
> On 10/04/2017 09:23 AM, Jan Beulich wrote:
>>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>>> --- a/tools/fuzz/x86_instruction_emulator/Makefile
>>> +++ b/tools/fuzz/x86_instruction_emulator/Makefile
>>> @@ -18,22 +18,22 @@ asm:
>>>  
>>>  asm/%: asm ;
>>>  
>>> -x86_emulate.c x86_emulate.h: %:
>>> +x86_emulate_user.c x86_emulate_user.h: %:
>> 
>> How about avoiding the names getting even longer? E.g. using
>> x86-emulate.[ch] or x86emul-user.[ch] instead?
> 
> My original idea was to make it easy to construct the original filename
> from the long filename.  I don't have super-strong opinions (mostly
> because I think all the options I've seen are pretty bad), but I still
> think that this is the least-bad option.
> 
> If you have strong opinions about one of the other ones, let me know and
> I'll change it.

Well, together with the suggested alternatives being shorter,
they also slightly improve word completion behavior when typing
in partial file names, so yes, I'd really appreciate renaming them
(and I've listed the suggestions above in the order of my
preference).

>>> @@ -42,7 +42,7 @@ all: x86-insn-fuzz-all
>>>  
>>>  .PHONY: distclean
>>>  distclean: clean
>>> -	rm -f x86_emulate x86_emulate.c x86_emulate.h asm
>>> +	rm -f x86_emulate x86_emulate_user.c x86_emulate_user.h asm
>> 
>> If you want to stick to the longer names, would you mind taking the
>> opportunity to make this just x86_emulate* ?
> 
> What if you put something in that directly called
> "x86_emulate_user.c.diff" (or something like that) and then ran "make
> clean"?
> 
> I tend to think that 'make clean' should only clean things that it is
> pretty confident were put there by the build system, and not the user.

Ah, yes, I see your point, albeit I don't fully agree: I would
actually prefer "make clean" to leave a clean tree, not one
with user created files left in. But indeed that's a matter of
taste.

Jan
George Dunlap Oct. 5, 2017, 1:50 p.m. UTC | #4 
On 10/05/2017 10:01 AM, Jan Beulich wrote:
>>>> On 04.10.17 at 18:34, <george.dunlap@citrix.com> wrote:
>> On 10/04/2017 09:23 AM, Jan Beulich wrote:
>>>>>> On 25.09.17 at 16:26, <george.dunlap@citrix.com> wrote:
>>>> --- a/tools/fuzz/x86_instruction_emulator/Makefile
>>>> +++ b/tools/fuzz/x86_instruction_emulator/Makefile
>>>> @@ -18,22 +18,22 @@ asm:
>>>>  
>>>>  asm/%: asm ;
>>>>  
>>>> -x86_emulate.c x86_emulate.h: %:
>>>> +x86_emulate_user.c x86_emulate_user.h: %:
>>>
>>> How about avoiding the names getting even longer? E.g. using
>>> x86-emulate.[ch] or x86emul-user.[ch] instead?
>>
>> My original idea was to make it easy to construct the original filename
>> from the long filename.  I don't have super-strong opinions (mostly
>> because I think all the options I've seen are pretty bad), but I still
>> think that this is the least-bad option.
>>
>> If you have strong opinions about one of the other ones, let me know and
>> I'll change it.
> 
> Well, together with the suggested alternatives being shorter,
> they also slightly improve word completion behavior when typing
> in partial file names, so yes, I'd really appreciate renaming them
> (and I've listed the suggestions above in the order of my
> preference).

Ok.

> 
>>>> @@ -42,7 +42,7 @@ all: x86-insn-fuzz-all
>>>>  
>>>>  .PHONY: distclean
>>>>  distclean: clean
>>>> -	rm -f x86_emulate x86_emulate.c x86_emulate.h asm
>>>> +	rm -f x86_emulate x86_emulate_user.c x86_emulate_user.h asm
>>>
>>> If you want to stick to the longer names, would you mind taking the
>>> opportunity to make this just x86_emulate* ?
>>
>> What if you put something in that directly called
>> "x86_emulate_user.c.diff" (or something like that) and then ran "make
>> clean"?
>>
>> I tend to think that 'make clean' should only clean things that it is
>> pretty confident were put there by the build system, and not the user.
> 
> Ah, yes, I see your point, albeit I don't fully agree: I would
> actually prefer "make clean" to leave a clean tree, not one
> with user created files left in. But indeed that's a matter of
> taste.

Well if that's the case we should have a whitelist, and do something
like "ls -a | (filter whitelist) | xargs rm -f".  But I think `git clean
-ffdx` does that job for most people these days.

 -George
diff mbox

Patch

diff --git a/tools/fuzz/x86_instruction_emulator/Makefile b/tools/fuzz/x86_instruction_emulator/Makefile
index a3f6b2c754..10009dc08f 100644
--- a/tools/fuzz/x86_instruction_emulator/Makefile
+++ b/tools/fuzz/x86_instruction_emulator/Makefile
@@ -18,22 +18,22 @@  asm:
 
 asm/%: asm ;
 
-x86_emulate.c x86_emulate.h: %:
+x86_emulate_user.c x86_emulate_user.h: %:
 	[ -L $* ] || ln -sf $(XEN_ROOT)/tools/tests/x86_emulator/$*
 
 CFLAGS += $(CFLAGS_xeninclude) -D__XEN_TOOLS__ -I.
 
 x86.h := asm/x86-vendors.h asm/x86-defns.h asm/msr-index.h
-x86_emulate.h := x86_emulate.h x86_emulate/x86_emulate.h $(x86.h)
+x86_emulate.h := x86_emulate_user.h x86_emulate/x86_emulate.h $(x86.h)
 
-x86_emulate.o: x86_emulate.c x86_emulate/x86_emulate.c $(x86_emulate.h)
+x86_emulate_user.o: x86_emulate_user.c x86_emulate/x86_emulate.c $(x86_emulate.h)
 
 fuzz-emul.o: $(x86_emulate.h)
 
-x86-insn-fuzzer.a: fuzz-emul.o x86_emulate.o
+x86-insn-fuzzer.a: fuzz-emul.o x86_emulate_user.o
 	$(AR) rc $@ $^
 
-afl-harness: afl-harness.o fuzz-emul.o x86_emulate.o
+afl-harness: afl-harness.o fuzz-emul.o x86_emulate_user.o
 	$(CC) $(CFLAGS) $^ -o $@
 
 # Common targets
@@ -42,7 +42,7 @@  all: x86-insn-fuzz-all
 
 .PHONY: distclean
 distclean: clean
-	rm -f x86_emulate x86_emulate.c x86_emulate.h asm
+	rm -f x86_emulate x86_emulate_user.c x86_emulate_user.h asm
 
 .PHONY: clean
 clean:
diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 92684cf088..dc180b070c 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -15,7 +15,7 @@ 
 #include <unistd.h>
 #include <xen/xen.h>
 
-#include "x86_emulate.h"
+#include "x86_emulate_user.h"
 
 #define MSR_INDEX_MAX 16
 
diff --git a/tools/tests/x86_emulator/Makefile b/tools/tests/x86_emulator/Makefile
index fd13ab53b1..888495a6a2 100644
--- a/tools/tests/x86_emulator/Makefile
+++ b/tools/tests/x86_emulator/Makefile
@@ -75,7 +75,7 @@  $(addsuffix .h,$(TESTCASES)): %.h: %.c testcase.mk Makefile
 $(addsuffix .c,$(SIMD)) $(addsuffix -avx.c,$(filter sse%,$(SIMD))):
 	ln -sf simd.c $@
 
-$(TARGET): x86_emulate.o test_x86_emulator.o
+$(TARGET): x86_emulate_user.o test_x86_emulator.o
 	$(HOSTCC) -o $@ $^
 
 .PHONY: clean
@@ -101,9 +101,9 @@  asm/%: asm ;
 HOSTCFLAGS += $(CFLAGS_xeninclude) -I.
 
 x86.h := asm/x86-vendors.h asm/x86-defns.h asm/msr-index.h
-x86_emulate.h := x86_emulate.h x86_emulate/x86_emulate.h $(x86.h)
+x86_emulate.h := x86_emulate_user.h x86_emulate/x86_emulate.h $(x86.h)
 
-x86_emulate.o: x86_emulate.c x86_emulate/x86_emulate.c $(x86_emulate.h)
+x86_emulate_user.o: x86_emulate_user.c x86_emulate/x86_emulate.c $(x86_emulate.h)
 	$(HOSTCC) $(HOSTCFLAGS) -D__XEN_TOOLS__ -c -g -o $@ $<
 
 test_x86_emulator.o: test_x86_emulator.c $(addsuffix .h,$(TESTCASES)) $(x86_emulate.h)
diff --git a/tools/tests/x86_emulator/test_x86_emulator.c b/tools/tests/x86_emulator/test_x86_emulator.c
index 4371e467e6..4d9c781610 100644
--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -3,7 +3,7 @@ 
 #include <stdio.h>
 #include <sys/mman.h>
 
-#include "x86_emulate.h"
+#include "x86_emulate_user.h"
 #include "blowfish.h"
 #include "sse.h"
 #include "sse2.h"
diff --git a/tools/tests/x86_emulator/x86_emulate.c b/tools/tests/x86_emulator/x86_emulate_user.c
similarity index 99%
rename from tools/tests/x86_emulator/x86_emulate.c
rename to tools/tests/x86_emulator/x86_emulate_user.c
index 79661d5c2b..adae6950c8 100644
--- a/tools/tests/x86_emulator/x86_emulate.c
+++ b/tools/tests/x86_emulator/x86_emulate_user.c
@@ -1,4 +1,4 @@ 
-#include "x86_emulate.h"
+#include "x86_emulate_user.h"
 
 #include <sys/mman.h>
 
diff --git a/tools/tests/x86_emulator/x86_emulate.h b/tools/tests/x86_emulator/x86_emulate_user.h
similarity index 100%
rename from tools/tests/x86_emulator/x86_emulate.h
rename to tools/tests/x86_emulator/x86_emulate_user.h