scripts: gen_syscalls: fix argument marshalling with 64-bit debug builds

Let's consider this (simplified) compilation result of a debug build
using -O0 for riscv64:

|__pinned_func
|static inline int k_sem_init(struct k_sem * sem,
|                             unsigned int initial_count,
|                             unsigned int limit)
|{
|    80000ad0:   6105                    addi    sp,sp,32
|    80000ad2:   ec06                    sd      ra,24(sp)
|    80000ad4:   e42a                    sd      a0,8(sp)
|    80000ad6:   c22e                    sw      a1,4(sp)
|    80000ad8:   c032                    sw      a2,0(sp)
|        ret = arch_is_user_context();
|    80000ada:   b39ff0ef                jal     ra,80000612
|        if (z_syscall_trap()) {
|    80000ade:   c911                    beqz    a0,80000af2
|                return (int) arch_syscall_invoke3(*(uintptr_t *)&sem,
|                                    *(uintptr_t *)&initial_count,
|                                    *(uintptr_t *)&limit,
|                                    K_SYSCALL_K_SEM_INIT);
|    80000ae0:   6522                    ld      a0,8(sp)
|    80000ae2:   00413583                ld      a1,4(sp)
|    80000ae6:   6602                    ld      a2,0(sp)
|    80000ae8:   0b700693                li      a3,183
|    [...]

We clearly see the 32-bit values `initial_count` (a1) and `limit` (a2)
being stored in memory with the `sw` (store word) instruction. Then,
according to the source code, the address of those values is casted
as a pointer to uintptr_t values, and that pointer is dereferenced to
get back those values with the `ld` (load double) instruction this time.

In other words, the assembly does exactly what the C code indicates.
This is wrong for 2 reasons:

- The top half of a1 and a2 will contain garbage due to the `ld` used
  to retrieve them. Whether or not the top bits will be cleared
  eventually depends on the architecture and compiler.
- Regardless of the above, a1 and a2 would be plain wrong on a big
  endian system.
- The load of a1 will cause a misaligned trap as it is 4-byte aligned
  while `ld` expects a 8-byte alignment.

The above code happens to work properly when compiling with
optimizations enabled as the compiler simplifies the cast and
dereference away, and register content is used as is in that case.
That doesn't make the code any more "correct" though.

The reason for taking the address of an argument and dereference it as an
uintptr_t pointer is most likely done to work around the fact that the
compiler refuses to cast an aggregate value to an integer, even if that
aggregate value is in fact a simple structure wrapping an integer.

So let's fix this code by:

- Removing the pointer dereference roundtrip and associated casts. This
  gets rid of all the issues listed above.
- Using a union to perform the type transition which deals with
  aggregates perfectly well. The compiler does optimize things to the
  same assembly output in the end.

This also makes the compiler happier as those pragmas to shut up warnings
are no longer needed. It should be the same about coverity.

Signed-off-by: Nicolas Pitre <[email protected]>

Author: Nicolas Pitre

scripts/gen_syscalls.py

@@ -61,8 +61,8 @@
 };
 """
 
-list_template = """
-/* auto-generated by gen_syscalls.py, don't edit */
+list_template = """/* auto-generated by gen_syscalls.py, don't edit */
+
 #ifndef ZEPHYR_SYSCALL_LIST_H
 #define ZEPHYR_SYSCALL_LIST_H
 
@@ -77,8 +77,8 @@
 #endif /* ZEPHYR_SYSCALL_LIST_H */
 """
 
-syscall_template = """
-/* auto-generated by gen_syscalls.py, don't edit */
+syscall_template = """/* auto-generated by gen_syscalls.py, don't edit */
+
 {include_guard}
 
 {tracing_include}
@@ -91,17 +91,6 @@
 #include <linker/sections.h>
 
 
-#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)
-#pragma GCC diagnostic push
-#endif
-
-#ifdef __GNUC__
-#pragma GCC diagnostic ignored "-Wstrict-aliasing"
-#if !defined(__XCC__)
-#pragma GCC diagnostic ignored "-Warray-bounds"
-#endif
-#endif
-
 #ifdef __cplusplus
 extern "C" {{
 #endif
@@ -112,10 +101,6 @@
 }}
 #endif
 
-#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)
-#pragma GCC diagnostic pop
-#endif
-
 #endif
 #endif /* include guard */
 """
@@ -196,25 +181,13 @@ def need_split(argtype):
 # Note: "lo" and "hi" are named in little endian conventions,
 # but it doesn't matter as long as they are consistently
 # generated.
-def union_decl(type):
-    return "union { struct { uintptr_t lo, hi; } split; %s val; }" % type
+def union_decl(type, split):
+    middle = "struct { uintptr_t lo, hi; } split" if split else "uintptr_t x"
+    return "union { %s; %s val; }" % (middle, type)
 
 def wrapper_defs(func_name, func_type, args, fn):
     ret64 = need_split(func_type)
     mrsh_args = [] # List of rvalue expressions for the marshalled invocation
-    split_args = []
-    nsplit = 0
-    for argtype, argname in args:
-        if need_split(argtype):
-            split_args.append((argtype, argname))
-            mrsh_args.append("parm%d.split.lo" % nsplit)
-            mrsh_args.append("parm%d.split.hi" % nsplit)
-            nsplit += 1
-        else:
-            mrsh_args.append("*(uintptr_t *)&" + argname)
-
-    if ret64:
-        mrsh_args.append("(uintptr_t)&ret64")
 
     decl_arglist = ", ".join([" ".join(argrec) for argrec in args]) or "void"
     syscall_id = "K_SYSCALL_" + func_name.upper()
@@ -228,10 +201,24 @@ def wrapper_defs(func_name, func_type, args, fn):
     wrap += ("\t" + "uint64_t ret64;\n") if ret64 else ""
     wrap += "\t" + "if (z_syscall_trap()) {\n"
 
-    for parmnum, rec in enumerate(split_args):
-        (argtype, argname) = rec
-        wrap += "\t\t%s parm%d;\n" % (union_decl(argtype), parmnum)
-        wrap += "\t\t" + "parm%d.val = %s;\n" % (parmnum, argname)
+    valist_args = []
+    for argnum, (argtype, argname) in enumerate(args):
+        split = need_split(argtype)
+        wrap += "\t\t%s parm%d" % (union_decl(argtype, split), argnum)
+        if argtype != "va_list":
+            wrap += " = { .val = %s };\n" % argname
+        else:
+            # va_list objects are ... peculiar.
+            wrap += ";\n" + "\t\t" + "va_copy(parm%d.val, %s);\n" % (argnum, argname)
+            valist_args.append("parm%d.val" % argnum)
+        if split:
+            mrsh_args.append("parm%d.split.lo" % argnum)
+            mrsh_args.append("parm%d.split.hi" % argnum)
+        else:
+            mrsh_args.append("parm%d.x" % argnum)
+
+    if ret64:
+        mrsh_args.append("(uintptr_t)&ret64")
 
     if len(mrsh_args) > 6:
         wrap += "\t\t" + "uintptr_t more[] = {\n"
@@ -243,21 +230,23 @@ def wrapper_defs(func_name, func_type, args, fn):
               % (len(mrsh_args),
                  ", ".join(mrsh_args + [syscall_id])))
 
-    # Coverity does not understand syscall mechanism
-    # and will already complain when any function argument
-    # is not of exact size as uintptr_t. So tell Coverity
-    # to ignore this particular rule here.
-    wrap += "\t\t/* coverity[OVERRUN] */\n"
-
     if ret64:
-        wrap += "\t\t" + "(void)%s;\n" % invoke
-        wrap += "\t\t" + "return (%s)ret64;\n" % func_type
+        invoke = "\t\t" + "(void) %s;\n" % invoke
+        retcode = "\t\t" + "return (%s) ret64;\n" % func_type
     elif func_type == "void":
-        wrap += "\t\t" + "%s;\n" % invoke
-        wrap += "\t\t" + "return;\n"
+        invoke = "\t\t" + "(void) %s;\n" % invoke
+        retcode = "\t\t" + "return;\n"
+    elif valist_args:
+        invoke = "\t\t" + "%s retval = %s;\n" % (func_type, invoke)
+        retcode = "\t\t" + "return retval;\n"
     else:
-        wrap += "\t\t" + "return (%s) %s;\n" % (func_type, invoke)
+        invoke = "\t\t" + "return (%s) %s;\n" % (func_type, invoke)
+        retcode = ""
 
+    wrap += invoke
+    for argname in valist_args:
+        wrap += "\t\t" + "va_end(%s);\n" % argname
+    wrap += retcode
     wrap += "\t" + "}\n"
     wrap += "#endif\n"
 
@@ -304,16 +293,11 @@ def marshall_defs(func_name, func_type, args):
     mrsh_name = "z_mrsh_" + func_name
 
     nmrsh = 0        # number of marshalled uintptr_t parameter
-    vrfy_parms = []  # list of (arg_num, mrsh_or_parm_num, bool_is_split)
-    split_parms = [] # list of a (arg_num, mrsh_num) for each split
-    for i, (argtype, _) in enumerate(args):
-        if need_split(argtype):
-            vrfy_parms.append((i, len(split_parms), True))
-            split_parms.append((i, nmrsh))
-            nmrsh += 2
-        else:
-            vrfy_parms.append((i, nmrsh, False))
-            nmrsh += 1
+    vrfy_parms = []  # list of (argtype, bool_is_split)
+    for (argtype, _) in args:
+        split = need_split(argtype)
+        vrfy_parms.append((argtype, split))
+        nmrsh += 2 if split else 1
 
     # Final argument for a 64 bit return value?
     if need_split(func_type):
@@ -337,23 +321,20 @@ def marshall_defs(func_name, func_type, args):
         mrsh += ("\tZ_OOPS(Z_SYSCALL_MEMORY_READ(more, "
                  + str(nmrsh - 5) + " * sizeof(uintptr_t)));\n")
 
-    for i, split_rec in enumerate(split_parms):
-        arg_num, mrsh_num = split_rec
-        arg_type = args[arg_num][0]
-        mrsh += "\t%s parm%d;\n" % (union_decl(arg_type), i)
-        mrsh += "\t" + "parm%d.split.lo = %s;\n" % (i, mrsh_rval(mrsh_num,
-                                                                 nmrsh))
-        mrsh += "\t" + "parm%d.split.hi = %s;\n" % (i, mrsh_rval(mrsh_num + 1,
-                                                                 nmrsh))
-    # Finally, invoke the verify function
-    out_args = []
-    for i, argn, is_split in vrfy_parms:
-        if is_split:
-            out_args.append("parm%d.val" % argn)
+    argnum = 0
+    for i, (argtype, split) in enumerate(vrfy_parms):
+        mrsh += "\t%s parm%d;\n" % (union_decl(argtype, split), i)
+        if split:
+            mrsh += "\t" + "parm%d.split.lo = %s;\n" % (i, mrsh_rval(argnum, nmrsh))
+            argnum += 1
+            mrsh += "\t" + "parm%d.split.hi = %s;\n" % (i, mrsh_rval(argnum, nmrsh))
         else:
-            out_args.append("*(%s*)&%s" % (args[i][0], mrsh_rval(argn, nmrsh)))
+            mrsh += "\t" + "parm%d.x = %s;\n" % (i, mrsh_rval(argnum, nmrsh))
+        argnum += 1
 
-    vrfy_call = "z_vrfy_%s(%s)\n" % (func_name, ", ".join(out_args))
+    # Finally, invoke the verify function
+    out_args = ", ".join(["parm%d.val" % i for i in range(len(args))])
+    vrfy_call = "z_vrfy_%s(%s)" % (func_name, out_args)
 
     if func_type == "void":
         mrsh += "\t" + "%s;\n" % vrfy_call
@@ -499,19 +480,10 @@ def main():
         mrsh_fn = os.path.join(args.base_output, fn + "_mrsh.c")
 
         with open(mrsh_fn, "w") as fp:
-            fp.write("/* auto-generated by gen_syscalls.py, don't edit */\n")
-            fp.write("#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)\n")
-            fp.write("#pragma GCC diagnostic push\n")
-            fp.write("#endif\n")
-            fp.write("#ifdef __GNUC__\n")
-            fp.write("#pragma GCC diagnostic ignored \"-Wstrict-aliasing\"\n")
-            fp.write("#endif\n")
+            fp.write("/* auto-generated by gen_syscalls.py, don't edit */\n\n")
             fp.write(mrsh_includes[fn] + "\n")
             fp.write("\n")
             fp.write(mrsh_defs[fn] + "\n")
-            fp.write("#if __GNUC__ > 4 || (__GNUC__ == 4 && __GNUC_MINOR__ >= 6)\n")
-            fp.write("#pragma GCC diagnostic pop\n")
-            fp.write("#endif\n")
 
 if __name__ == "__main__":
     main()