Re: [Qemu-devel] [PATCH] trace: only permit standard C types and fixed size integer types

[Philippe Mathieu-Daudé]

On 03/06/2018 12:46 PM, Daniel P. Berrangé wrote:
> Some trace backends will compile code based on the declared trace
> events. It should not be assumed that the backends can resolve any QEMU
> specific typedefs. So trace events should restrict their argument
> types to the standard C types and fixed size integer types. Any complex
> pointer types can be declared as "void *" for purposes of trace events,
> since nothing will be dereferencing these pointer arguments.

Exactly what I was thinking of!

> 
> Signed-off-by: Daniel P. Berrangé <address@hidden>
> ---
>  scripts/tracetool/__init__.py | 45 
> +++++++++++++++++++++++++++++++++++++++++++
>  trace-events                  |  6 +++---

Maybe split in 2 different patches?

>  2 files changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/tracetool/__init__.py b/scripts/tracetool/__init__.py
> index 3646c2b9fc..52cc687ae3 100644
> --- a/scripts/tracetool/__init__.py
> +++ b/scripts/tracetool/__init__.py
> @@ -41,6 +41,50 @@ def out(*lines, **kwargs):
>      lines = [ l % kwargs for l in lines ]
>      sys.stdout.writelines("\n".join(lines) + "\n")
>  
> +# We only want to allow standard C types or fixed sized
> +# integer types. We don't want QEMU specific types
> +# as we can't assume trace backends can resolve all the
> +# typedefs
> +ALLOWED_TYPES = [
> +    "int",
> +    "long",
> +    "short",
> +    "char",
> +    "bool",
> +    "unsigned",
> +    "signed",
> +    "float",
> +    "double",
> +    "int8_t",
> +    "uint8_t",
> +    "int16_t",
> +    "uint16_t",
> +    "int32_t",
> +    "uint32_t",
> +    "int64_t",
> +    "uint64_t",
> +    "void",
> +    "size_t",
> +    "ssize_t",
> +    "uintptr_t",

ptrdiff_t?

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>

> +    # Magic substitution is done by tracetool
> +    "TCGv",
> +]
> +
> +def validate_type(name):
> +    bits = name.split(" ")
> +    for bit in bits:
> +        bit = re.sub("\*", "", bit)
> +        if bit == "":
> +            continue
> +        if bit == "const":
> +            continue
> +        if bit not in ALLOWED_TYPES:
> +            raise ValueError("Argument type '%s' is not in whitelist. "
> +                             "Only standard C types and fixed size integer "
> +                             "types should be used. struct, union, and "
> +                             "other complex pointer types should be "
> +                             "declared as 'void *'" % name)
>  
>  class Arguments:
>      """Event arguments description."""
> @@ -87,6 +131,7 @@ class Arguments:
>              else:
>                  arg_type, identifier = arg.rsplit(None, 1)
>  
> +            validate_type(arg_type)
>              res.append((arg_type, identifier))
>          return Arguments(res)
>  
> diff --git a/trace-events b/trace-events
> index 89fcad0fd1..855b0ab240 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -68,9 +68,9 @@ memory_region_tb_read(int cpu_index, uint64_t addr, 
> uint64_t value, unsigned siz
>  memory_region_tb_write(int cpu_index, uint64_t addr, uint64_t value, 
> unsigned size) "cpu %d addr 0x%"PRIx64" value 0x%"PRIx64" size %u"
>  memory_region_ram_device_read(int cpu_index, void *mr, uint64_t addr, 
> uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 
> 0x%"PRIx64" size %u"
>  memory_region_ram_device_write(int cpu_index, void *mr, uint64_t addr, 
> uint64_t value, unsigned size) "cpu %d mr %p addr 0x%"PRIx64" value 
> 0x%"PRIx64" size %u"
> -flatview_new(FlatView *view, MemoryRegion *root) "%p (root %p)"
> -flatview_destroy(FlatView *view, MemoryRegion *root) "%p (root %p)"
> -flatview_destroy_rcu(FlatView *view, MemoryRegion *root) "%p (root %p)"
> +flatview_new(void *view, void *root) "%p (root %p)"
> +flatview_destroy(void *view, void *root) "%p (root %p)"
> +flatview_destroy_rcu(void *view, void *root) "%p (root %p)"
>  
>  # gdbstub.c
>  gdbstub_op_start(const char *device) "Starting gdbstub using device %s"
>