[Semihosting Tests PATCH v2 0/3] add SYS_GET_CMDLINE test

[Semihosting Tests PATCH v2 0/3] add SYS_GET_CMDLINE test

[Alex Bennée]

Hi Peter,

Looking at bug #2322 I wanted to make sure SYS_GET_CMDLINE works as I
expected. While at it I needed to fix a compile error with headers
which I guess we got away with on earlier compilers.

I've added an editorconfig for good measure.

v2
  - addressed review comments

Alex Bennée (3):
  .editorconfig: add code conventions for tooling
  update includes for bare metal compiling
  add SYS_GET_CMDLINE test

 .editorconfig | 29 +++++++++++++++++++++++++++++
 Makefile      | 22 +++++++++++-----------
 semihosting.c |  4 ++--
 semihosting.h |  2 +-
 string.c      |  2 +-
 usertest.c    | 33 ++++++++++++++++++++++++++++++++-
 6 files changed, 76 insertions(+), 16 deletions(-)
 create mode 100644 .editorconfig

-- 
2.39.2

[Semihosting Tests PATCH v2 1/3] .editorconfig: add code conventions for tooling

[Alex Bennée]

It's a pain when you come back to a code base you haven't touched in a
while and realise whatever indent settings you were using having
carried over. Add an editorconfig and be done with it.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - drop mention of custom major modes (not needed here)
  - include section for assembly
---
 .editorconfig | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)
 create mode 100644 .editorconfig

diff --git a/.editorconfig b/.editorconfig
new file mode 100644
index 0000000..c72a55c
--- /dev/null
+++ b/.editorconfig
@@ -0,0 +1,29 @@
+# EditorConfig is a file format and collection of text editor plugins
+# for maintaining consistent coding styles between different editors
+# and IDEs. Most popular editors support this either natively or via
+# plugin.
+#
+# Check https://editorconfig.org for details.
+#
+
+root = true
+
+[*]
+end_of_line = lf
+insert_final_newline = true
+charset = utf-8
+
+[Makefile*]
+indent_style = tab
+indent_size = 8
+emacs_mode = makefile
+
+[*.{c,h}]
+indent_style = space
+indent_size = 4
+emacs_mode = c
+
+[*.{s,S}]
+indent_style = tab
+indent_size = 8
+emacs_mode = asm
-- 
2.39.2

Re: [Semihosting Tests PATCH v2 1/3] .editorconfig: add code conventions for tooling

[Brian Cain]

On 5/30/2024 6:23 AM, Alex Bennée wrote:
> It's a pain when you come back to a code base you haven't touched in a
> while and realise whatever indent settings you were using having
> carried over. Add an editorconfig and be done with it.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>


Adding an editorconfig seems like a great idea IMO.  But I wonder - will 
it result in unintentional additional changes when saving a file that 
contains baseline non-conformance?

Related: would a .clang-format file also be useful? git-clang-format can 
be used to apply formatting changes only on the code that's been changed.

Also: should we consider excluding any exceptional files that we don't 
expect to conform?


> ---
> v2
>    - drop mention of custom major modes (not needed here)
>    - include section for assembly
> ---
>   .editorconfig | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
>   create mode 100644 .editorconfig
>
> diff --git a/.editorconfig b/.editorconfig
> new file mode 100644
> index 0000000..c72a55c
> --- /dev/null
> +++ b/.editorconfig
> @@ -0,0 +1,29 @@
> +# EditorConfig is a file format and collection of text editor plugins
> +# for maintaining consistent coding styles between different editors
> +# and IDEs. Most popular editors support this either natively or via
> +# plugin.
> +#
> +# Check https://editorconfig.org for details.
> +#
> +
> +root = true
> +
> +[*]
> +end_of_line = lf
> +insert_final_newline = true
> +charset = utf-8
> +
> +[Makefile*]
> +indent_style = tab
> +indent_size = 8
> +emacs_mode = makefile
> +
> +[*.{c,h}]
> +indent_style = space
> +indent_size = 4
> +emacs_mode = c
> +
> +[*.{s,S}]
> +indent_style = tab
> +indent_size = 8
> +emacs_mode = asm

Re: [Semihosting Tests PATCH v2 1/3] .editorconfig: add code conventions for tooling

[Alex Bennée]

Brian Cain <quic_bcain@quicinc.com> writes:

> On 5/30/2024 6:23 AM, Alex Bennée wrote:
>> It's a pain when you come back to a code base you haven't touched in a
>> while and realise whatever indent settings you were using having
>> carried over. Add an editorconfig and be done with it.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
>
> Adding an editorconfig seems like a great idea IMO.  But I wonder -
> will it result in unintentional additional changes when saving a file
> that contains baseline non-conformance?

This is for the semihosting tests, we have had an editorconfig in the
mainline QEMU repo for a long time. Generally it's just standardising
what people usually hand configure their editors for which can range in
their aggressiveness in reformatting existing code.

> Related: would a .clang-format file also be useful? git-clang-format
> can be used to apply formatting changes only on the code that's been
> changed.

As a pre-commit hook? Or via something like clangd? 

> Also: should we consider excluding any exceptional files that we don't
> expect to conform?

Do we have such files? We certainly have a bunch of legacy whitespace
damage hanging about but I didn't think we had a lot of non-conforming
files.

<snip>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

Re: [Semihosting Tests PATCH v2 1/3] .editorconfig: add code conventions for tooling

[Peter Maydell]

On Fri, 31 May 2024 at 09:54, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Brian Cain <quic_bcain@quicinc.com> writes:
> > Related: would a .clang-format file also be useful? git-clang-format
> > can be used to apply formatting changes only on the code that's been
> > changed.
>
> As a pre-commit hook? Or via something like clangd?

I think last time somebody looked at clangd it wasn't quite
flexible enough to format code to QEMU's style preferences.
But that was some years ago, so I might be misremembering or
the situation might have changed.

In terms of project consensus, I think "here's tooling/config
you can use to follow our formatting preferences if you like"
is probably a better place to start than anything that is
an automatically-applied-by-default check.

(For the semihosting-tests I wouldn't bother, because they
get almost no contributions: 8 commits in the last 5 years.)

thanks
-- PMM

[Semihosting Tests PATCH v2 2/3] update includes for bare metal compiling

[Alex Bennée]

We shouldn't use <string.h> for our own implementation. Also the base
types we need live in <stdint.h> as <inttypes.h> doesn't exist for the
bare metal compilers.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 semihosting.c | 4 ++--
 semihosting.h | 2 +-
 string.c      | 2 +-
 usertest.c    | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/semihosting.c b/semihosting.c
index 7a0ee0c..27dafbe 100644
--- a/semihosting.c
+++ b/semihosting.c
@@ -16,8 +16,8 @@
  *    from this software without specific prior written permission.
  */
 
-#include <inttypes.h>
-#include <string.h>
+#include <stdint.h>
+#include "string.h"
 #include "semihosting.h"
 
 int semi_open(char const *filename, int mode)
diff --git a/semihosting.h b/semihosting.h
index 06cda8d..68344fb 100644
--- a/semihosting.h
+++ b/semihosting.h
@@ -19,7 +19,7 @@
 #ifndef SEMIHOSTING_H
 #define SEMIHOSTING_H
 
-#include <inttypes.h>
+#include <stdint.h>
 
 #define SYS_OPEN	1
 #define OPEN_RDONLY	1
diff --git a/string.c b/string.c
index 045d71b..c289aa1 100644
--- a/string.c
+++ b/string.c
@@ -16,7 +16,7 @@
  *    from this software without specific prior written permission.
  */
 
-#include <string.h>
+#include "string.h"
 
 static void *__memmove_down(void *__dest, __const void *__src, size_t __n)
 {
diff --git a/usertest.c b/usertest.c
index ce0f61d..5df95f3 100644
--- a/usertest.c
+++ b/usertest.c
@@ -20,7 +20,7 @@
  *    from this software without specific prior written permission.
  */
 
-#include <stdio.h>
+#include "string.h"
 #include "semihosting.h"
 #include "printf/printf.h"
 
-- 
2.39.2

[Semihosting Tests PATCH v2 3/3] add SYS_GET_CMDLINE test

[Alex Bennée]

Adds a new test case for the semihosting SYS_GET_CMDLINE call. We can
use the existing previously unused semi_get_cmdline helper and we then
verify:

 - the call succeeds
 - the call returns the expected length string
 - the call returns the expected binary name as arg0

The most fiddly bit was exposing the binary name into the program so
we can validate the result.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---
v2
  - add return leg on failing test
  - check returned length is sane
  - s/could/couldn't/
  - just let array reference decay to the pointer
  - rewrite the commit message to be less terse
---
 Makefile   | 22 +++++++++++-----------
 usertest.c | 31 +++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 11 deletions(-)

diff --git a/Makefile b/Makefile
index 59fd831..f77665f 100644
--- a/Makefile
+++ b/Makefile
@@ -85,37 +85,37 @@ systest-srcs = start.S string.c $(usertest-srcs)
 microbit-systest-srcs = start-microbit.S string.c $(usertest-srcs)
 
 usertest-a32: $(usertest-srcs)
-	$(A32GCC) --static -o $@ $^
+	$(A32GCC) -DBINARY_NAME="\"$@\"" --static -o $@ $^
 
 usertest-t32: $(usertest-srcs)
-	$(T32GCC) --static -o $@ $^
+	$(T32GCC) -DBINARY_NAME="\"$@\"" --static -o $@ $^
 
 usertest-a32-hlt: $(usertest-srcs)
-	$(A32GCC) -DUSE_HLT --static -o $@ $^
+	$(A32GCC) -DBINARY_NAME="\"$@\"" -DUSE_HLT --static -o $@ $^
 
 usertest-t32-hlt: $(usertest-srcs)
-	$(T32GCC) -DUSE_HLT --static -o $@ $^
+	$(T32GCC) -DBINARY_NAME="\"$@\"" -DUSE_HLT --static -o $@ $^
 
 usertest-a64: $(usertest-srcs)
-	$(A64GCC) --static -o $@ $^
+	$(A64GCC) -DBINARY_NAME="\"$@\"" --static -o $@ $^
 
 systest-a32.axf: $(systest-srcs)
-	$(A32GCC) -o $@ $^ $(A32LINKOPTS)
+	$(A32GCC) -DBINARY_NAME="\"$@\"" -o $@ $^ $(A32LINKOPTS)
 
 systest-t32.axf: $(systest-srcs)
-	$(T32GCC) -o $@ $^ $(A32LINKOPTS)
+	$(T32GCC) -DBINARY_NAME="\"$@\"" -o $@ $^ $(A32LINKOPTS)
 
 systest-a32-hlt.axf: $(systest-srcs)
-	$(A32GCC) -DUSE_HLT -o $@ $^ $(A32LINKOPTS)
+	$(A32GCC) -DBINARY_NAME="\"$@\"" -DUSE_HLT -o $@ $^ $(A32LINKOPTS)
 
 systest-t32-hlt.axf: $(systest-srcs)
-	$(T32GCC) -DUSE_HLT -o $@ $^ $(A32LINKOPTS)
+	$(T32GCC) -DBINARY_NAME="\"$@\"" -DUSE_HLT -o $@ $^ $(A32LINKOPTS)
 
 systest-t32-bkpt.axf: $(microbit-systest-srcs)
-	$(V7MGCC) -DUSE_BKPT -o $@ $^ $(AV7MLINKOPTS)
+	$(V7MGCC) -DBINARY_NAME="\"$@\"" -DUSE_BKPT -o $@ $^ $(AV7MLINKOPTS)
 
 systest-a64.axf: $(systest-srcs)
-	$(A64GCC) -nostdlib -o $@ $^ $(A64LINKOPTS)
+	$(A64GCC) -DBINARY_NAME="\"$@\"" -nostdlib -o $@ $^ $(A64LINKOPTS)
 
 run-usertest-a32: usertest-a32
 	$(QEMU_ARM) usertest-a32
diff --git a/usertest.c b/usertest.c
index 5df95f3..7a12896 100644
--- a/usertest.c
+++ b/usertest.c
@@ -315,6 +315,33 @@ static int test_feature_detect(void)
     return 0;
 }
 
+static int test_cmdline(void)
+{
+    char cmdline[256];
+    int actual;
+    const char *s, *c;
+
+    if (semi_get_cmdline(cmdline, sizeof(cmdline), &actual)) {
+        semi_write0("FAIL couldn't recover command line\n");
+        return 1;
+    }
+
+    if (strlen(BINARY_NAME) != actual) {
+        semi_write0("FAIL cmdline length not what expected: ");
+        semi_write0(cmdline);
+        return 1;
+    }
+
+    if (strcmp(cmdline, BINARY_NAME) != 0) {
+        semi_write0("FAIL unexpected command line: ");
+        semi_write0(cmdline);
+        return 1;
+    }
+
+    semi_write0("PASS command line test\n");
+    return 0;
+}
+
 int main(void)
 {
     void *bufp;
@@ -366,6 +393,10 @@ int main(void)
         return 1;
     }
 
+    if (test_cmdline()) {
+        return 1;
+    }
+
     semi_write0("ALL TESTS PASSED\n");
 
     /* If we have EXIT_EXTENDED then use it */
-- 
2.39.2

Re: [Semihosting Tests PATCH v2 0/3] add SYS_GET_CMDLINE test

[Peter Maydell]

On Thu, 30 May 2024 at 12:23, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Hi Peter,
>
> Looking at bug #2322 I wanted to make sure SYS_GET_CMDLINE works as I
> expected. While at it I needed to fix a compile error with headers
> which I guess we got away with on earlier compilers.
>
> I've added an editorconfig for good measure.
>
> v2
>   - addressed review comments
>
> Alex Bennée (3):
>   .editorconfig: add code conventions for tooling
>   update includes for bare metal compiling
>   add SYS_GET_CMDLINE test

Applied to the semihosting-tests git repo, thanks.

-- PMM