[PATCH 0/1] RISC-V: Improve handling of mapping symbols with dot suffix

[PATCH 0/1] RISC-V: Improve handling of mapping symbols with dot suffix

[Tsukasa OI]

Hi,

Commit 9326300e4d3dbe943380b30766ed474d98df07ba ("RISC-V: Add support for
numbered ISA mapping strings") is required but there's a room for
improvement.  The reason I'm submitting this is because I could not accept
a misuse of "int" (size_t would be valid) but decided to make other minor
improvements (allocation size, performance and better variable names).

Tsukasa




Tsukasa OI (1):
  RISC-V: Improve handling of mapping symbols with dot suffix

 opcodes/riscv-dis.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)


base-commit: 5e2c9ce9c0bf4763a6d17a3a5bee9011ec710f10
-- 
2.42.0

[PATCH 1/1] RISC-V: Improve handling of mapping symbols with dot suffix

[Tsukasa OI]

From: Tsukasa OI <research_trasio@irq.a4lg.com>

This commit makes minor improvements to mapping symbols (executable)
handling with a dot suffix.

1.  Use size_t instead of int
2.  Allocate minimum size for the architectural string buffer.
3.  memcpy instead of strncpy because we know the exact size to copy.
4.  Minor variable naming changes.

opcodes/ChangeLog:

	* riscv-dis.c (riscv_get_map_state): Minor improvements to
	handling of executable mapping symbols with dot suffix.
---
 opcodes/riscv-dis.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
index 216916e9426d..18547d81c20d 100644
--- a/opcodes/riscv-dis.c
+++ b/opcodes/riscv-dis.c
@@ -875,12 +875,12 @@ riscv_get_map_state (int n,
       char *suffix = strchr (name, '.');
       if (suffix)
 	{
-	  int suffix_index = (int)(suffix - name);
-	  char *name_substr = xmalloc (suffix_index + 1);
-	  strncpy (name_substr, name, suffix_index);
-	  name_substr[suffix_index] = '\0';
-	  riscv_parse_subset (&riscv_rps_dis, name_substr + 2);
-	  free (name_substr);
+	  size_t arch_len = (size_t) (suffix - name) - 2;
+	  char *arch = xmalloc (arch_len + 1);
+	  memcpy (arch, name + 2, arch_len);
+	  arch[arch_len] = '\0';
+	  riscv_parse_subset (&riscv_rps_dis, arch);
+	  free (arch);
 	}
       else
 	riscv_parse_subset (&riscv_rps_dis, name + 2);
-- 
2.42.0

Re: [PATCH 1/1] RISC-V: Improve handling of mapping symbols with dot suffix

[Nelson Chu]

[-- Attachment #1: Type: text/plain, Size: 1934 bytes --]

Okay, use size_t looks good.  But I don't suggest changing other
developers' variant naming.  This is usually not a real matter, especially
the naming is already clear enough to understand.

Thanks
Nelson

On Sat, Oct 14, 2023 at 4:37 PM Tsukasa OI <research_trasio@irq.a4lg.com>
wrote:

> From: Tsukasa OI <research_trasio@irq.a4lg.com>
>
> This commit makes minor improvements to mapping symbols (executable)
> handling with a dot suffix.
>
> 1.  Use size_t instead of int
> 2.  Allocate minimum size for the architectural string buffer.
> 3.  memcpy instead of strncpy because we know the exact size to copy.
> 4.  Minor variable naming changes.
>
> opcodes/ChangeLog:
>
>         * riscv-dis.c (riscv_get_map_state): Minor improvements to
>         handling of executable mapping symbols with dot suffix.
> ---
>  opcodes/riscv-dis.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
> index 216916e9426d..18547d81c20d 100644
> --- a/opcodes/riscv-dis.c
> +++ b/opcodes/riscv-dis.c
> @@ -875,12 +875,12 @@ riscv_get_map_state (int n,
>        char *suffix = strchr (name, '.');
>        if (suffix)
>         {
> -         int suffix_index = (int)(suffix - name);
> -         char *name_substr = xmalloc (suffix_index + 1);
> -         strncpy (name_substr, name, suffix_index);
> -         name_substr[suffix_index] = '\0';
> -         riscv_parse_subset (&riscv_rps_dis, name_substr + 2);
> -         free (name_substr);
> +         size_t arch_len = (size_t) (suffix - name) - 2;
> +         char *arch = xmalloc (arch_len + 1);
> +         memcpy (arch, name + 2, arch_len);
> +         arch[arch_len] = '\0';
> +         riscv_parse_subset (&riscv_rps_dis, arch);
> +         free (arch);
>         }
>        else
>         riscv_parse_subset (&riscv_rps_dis, name + 2);
> --
> 2.42.0
>
>

Re: [PATCH 1/1] RISC-V: Improve handling of mapping symbols with dot suffix

[Tsukasa OI]

On 2023/10/15 11:38, Nelson Chu wrote:
> Okay, use size_t looks good.  But I don't suggest changing other
> developers' variant naming.  This is usually not a real matter,
> especially the naming is already clear enough to understand.
> 
> Thanks
> Nelson

Well, normally I wouldn't touch that much (if there's only 1. as listed
below) but because I found two possible improvements to the existing
code (2. and 3. as below), I felt that keeping the variable names less
important.

By other changes, "suffix_index" no longer fits the context.
"name_substr" is still valid because this is still a substring of
"name". But since this patch separates *only* the ISA string, just
renaming it "arch" felt clearer.

Thanks,
Tsukasa

> 
> On Sat, Oct 14, 2023 at 4:37 PM Tsukasa OI <research_trasio@irq.a4lg.com
> <mailto:research_trasio@irq.a4lg.com>> wrote:
> 
>     From: Tsukasa OI <research_trasio@irq.a4lg.com
>     <mailto:research_trasio@irq.a4lg.com>>
> 
>     This commit makes minor improvements to mapping symbols (executable)
>     handling with a dot suffix.
> 
>     1.  Use size_t instead of int
>     2.  Allocate minimum size for the architectural string buffer.
>     3.  memcpy instead of strncpy because we know the exact size to copy.
>     4.  Minor variable naming changes.
> 
>     opcodes/ChangeLog:
> 
>             * riscv-dis.c (riscv_get_map_state): Minor improvements to
>             handling of executable mapping symbols with dot suffix.
>     ---
>      opcodes/riscv-dis.c | 12 ++++++------
>      1 file changed, 6 insertions(+), 6 deletions(-)
> 
>     diff --git a/opcodes/riscv-dis.c b/opcodes/riscv-dis.c
>     index 216916e9426d..18547d81c20d 100644
>     --- a/opcodes/riscv-dis.c
>     +++ b/opcodes/riscv-dis.c
>     @@ -875,12 +875,12 @@ riscv_get_map_state (int n,
>            char *suffix = strchr (name, '.');
>            if (suffix)
>             {
>     -         int suffix_index = (int)(suffix - name);
>     -         char *name_substr = xmalloc (suffix_index + 1);
>     -         strncpy (name_substr, name, suffix_index);
>     -         name_substr[suffix_index] = '\0';
>     -         riscv_parse_subset (&riscv_rps_dis, name_substr + 2);
>     -         free (name_substr);
>     +         size_t arch_len = (size_t) (suffix - name) - 2;
>     +         char *arch = xmalloc (arch_len + 1);
>     +         memcpy (arch, name + 2, arch_len);
>     +         arch[arch_len] = '\0';
>     +         riscv_parse_subset (&riscv_rps_dis, arch);
>     +         free (arch);
>             }
>            else
>             riscv_parse_subset (&riscv_rps_dis, name + 2);
>     -- 
>     2.42.0
>