Re: [PATCH v3 16/24] disas/nanomips: Remove IMMEDIATE functions
On 1/11/22 12:28, Stefan Weil wrote:
Am 01.11.22 um 10:27 schrieb Philippe Mathieu-Daudé:
On 1/11/22 09:28, Stefan Weil via wrote:
Am 12.09.22 um 14:26 schrieb Milica Lazarevic:
Both versions of IMMEDIATE functions have been removed.
Before this patch, we'd been calling img_format twice, the first time
through the IMMEDIATE to get an appropriate string and the second time
to print that string. There's no more need for that. Therefore,
calls to
IMMEDIATE are removed, and now we're directly printing the integer
values instead.
Signed-off-by: Milica Lazarevic
---
disas/nanomips.cpp | 756
-
1 file changed, 265 insertions(+), 491 deletions(-)
diff --git a/disas/nanomips.cpp b/disas/nanomips.cpp
index 816155527d..441204bb84 100644
--- a/disas/nanomips.cpp
+++ b/disas/nanomips.cpp
[...]
@@ -3305,11 +3271,9 @@ static char *CACHE(uint64 instruction,
Dis_info *info)
uint64 rs_value = extract_rs_20_19_18_17_16(instruction);
int64 s_value = extract_s__se8_15_7_6_5_4_3_2_1_0(instruction);
- char *op = IMMEDIATE(op_value);
- char *s = IMMEDIATE(s_value);
const char *rs = GPR(rs_value);
- return img_format("CACHE %s, %s(%s)", op, s, rs);
+ return img_format("CACHE 0x%" PRIx64 ", %s(%s)", op_value,
s_value, rs);
}
@@ -3329,11 +3293,9 @@ static char *CACHEE(uint64 instruction,
Dis_info *info)
uint64 rs_value = extract_rs_20_19_18_17_16(instruction);
int64 s_value = extract_s__se8_15_7_6_5_4_3_2_1_0(instruction);
- char *op = IMMEDIATE(op_value);
- char *s = IMMEDIATE(s_value);
const char *rs = GPR(rs_value);
- return img_format("CACHEE %s, %s(%s)", op, s, rs);
+ return img_format("CACHEE 0x%" PRIx64 ", %s(%s)", op_value,
s_value, rs);
}
Do we really want to format "int64 s_value" as a string? The code now
has lots of wrong format strings. Add the patch below to get the
compiler report.
We once had a discussion about using G_GNUC_PRINTF for local
functions or not. I think that this example clearly shows that it
should be mandatory.
Yes. The problem here is nobody wants to maintain this code, but we
inherited it. IIUC this series doesn't make it worst, it just remove
the C++ dependency on UNIX-based hosts.
I expect that "%s" with an int64 s_value will cause a crash while the
old code worked, so things are worse now and should be fixed for the
release. If nobody maintains that code, I can try to prepare a patch.
Well since I merged I already prepared some patches to fix this.
Re: [PATCH v3 16/24] disas/nanomips: Remove IMMEDIATE functions
Am 01.11.22 um 10:27 schrieb Philippe Mathieu-Daudé:
On 1/11/22 09:28, Stefan Weil via wrote:
Am 12.09.22 um 14:26 schrieb Milica Lazarevic:
Both versions of IMMEDIATE functions have been removed.
Before this patch, we'd been calling img_format twice, the first time
through the IMMEDIATE to get an appropriate string and the second time
to print that string. There's no more need for that. Therefore,
calls to
IMMEDIATE are removed, and now we're directly printing the integer
values instead.
Signed-off-by: Milica Lazarevic
---
disas/nanomips.cpp | 756
-
1 file changed, 265 insertions(+), 491 deletions(-)
diff --git a/disas/nanomips.cpp b/disas/nanomips.cpp
index 816155527d..441204bb84 100644
--- a/disas/nanomips.cpp
+++ b/disas/nanomips.cpp
[...]
@@ -3305,11 +3271,9 @@ static char *CACHE(uint64 instruction,
Dis_info *info)
uint64 rs_value = extract_rs_20_19_18_17_16(instruction);
int64 s_value = extract_s__se8_15_7_6_5_4_3_2_1_0(instruction);
- char *op = IMMEDIATE(op_value);
- char *s = IMMEDIATE(s_value);
const char *rs = GPR(rs_value);
- return img_format("CACHE %s, %s(%s)", op, s, rs);
+ return img_format("CACHE 0x%" PRIx64 ", %s(%s)", op_value,
s_value, rs);
}
@@ -3329,11 +3293,9 @@ static char *CACHEE(uint64 instruction,
Dis_info *info)
uint64 rs_value = extract_rs_20_19_18_17_16(instruction);
int64 s_value = extract_s__se8_15_7_6_5_4_3_2_1_0(instruction);
- char *op = IMMEDIATE(op_value);
- char *s = IMMEDIATE(s_value);
const char *rs = GPR(rs_value);
- return img_format("CACHEE %s, %s(%s)", op, s, rs);
+ return img_format("CACHEE 0x%" PRIx64 ", %s(%s)", op_value,
s_value, rs);
}
Do we really want to format "int64 s_value" as a string? The code now
has lots of wrong format strings. Add the patch below to get the
compiler report.
We once had a discussion about using G_GNUC_PRINTF for local
functions or not. I think that this example clearly shows that it
should be mandatory.
Yes. The problem here is nobody wants to maintain this code, but we
inherited it. IIUC this series doesn't make it worst, it just remove
the C++ dependency on UNIX-based hosts.
I expect that "%s" with an int64 s_value will cause a crash while the
old code worked, so things are worse now and should be fixed for the
release. If nobody maintains that code, I can try to prepare a patch.
Stefan
Re: [PATCH v3 16/24] disas/nanomips: Remove IMMEDIATE functions
On 1/11/22 09:28, Stefan Weil via wrote:
Am 12.09.22 um 14:26 schrieb Milica Lazarevic:
Both versions of IMMEDIATE functions have been removed.
Before this patch, we'd been calling img_format twice, the first time
through the IMMEDIATE to get an appropriate string and the second time
to print that string. There's no more need for that. Therefore, calls to
IMMEDIATE are removed, and now we're directly printing the integer
values instead.
Signed-off-by: Milica Lazarevic
---
disas/nanomips.cpp | 756 -
1 file changed, 265 insertions(+), 491 deletions(-)
diff --git a/disas/nanomips.cpp b/disas/nanomips.cpp
index 816155527d..441204bb84 100644
--- a/disas/nanomips.cpp
+++ b/disas/nanomips.cpp
[...]
@@ -3305,11 +3271,9 @@ static char *CACHE(uint64 instruction, Dis_info
*info)
uint64 rs_value = extract_rs_20_19_18_17_16(instruction);
int64 s_value = extract_s__se8_15_7_6_5_4_3_2_1_0(instruction);
- char *op = IMMEDIATE(op_value);
- char *s = IMMEDIATE(s_value);
const char *rs = GPR(rs_value);
- return img_format("CACHE %s, %s(%s)", op, s, rs);
+ return img_format("CACHE 0x%" PRIx64 ", %s(%s)", op_value,
s_value, rs);
}
@@ -3329,11 +3293,9 @@ static char *CACHEE(uint64 instruction,
Dis_info *info)
uint64 rs_value = extract_rs_20_19_18_17_16(instruction);
int64 s_value = extract_s__se8_15_7_6_5_4_3_2_1_0(instruction);
- char *op = IMMEDIATE(op_value);
- char *s = IMMEDIATE(s_value);
const char *rs = GPR(rs_value);
- return img_format("CACHEE %s, %s(%s)", op, s, rs);
+ return img_format("CACHEE 0x%" PRIx64 ", %s(%s)", op_value,
s_value, rs);
}
Do we really want to format "int64 s_value" as a string? The code now
has lots of wrong format strings. Add the patch below to get the
compiler report.
We once had a discussion about using G_GNUC_PRINTF for local functions
or not. I think that this example clearly shows that it should be
mandatory.
Yes. The problem here is nobody wants to maintain this code, but we
inherited it. IIUC this series doesn't make it worst, it just remove
the C++ dependency on UNIX-based hosts.
Regards,
Stefan
diff --git a/disas/nanomips.c b/disas/nanomips.c
index 9647f1a8e3..c875818cb9 100644
--- a/disas/nanomips.c
+++ b/disas/nanomips.c
@@ -95,7 +95,7 @@ typedef struct Pool {
#define IMGASSERTONCE(test)
-static char *img_format(const char *format, ...)
+static char * G_GNUC_PRINTF(1, 2) img_format(const char *format, ...)
{
char *buffer;
va_list args;
Re: [PATCH v3 16/24] disas/nanomips: Remove IMMEDIATE functions
Am 12.09.22 um 14:26 schrieb Milica Lazarevic:
Both versions of IMMEDIATE functions have been removed.
Before this patch, we'd been calling img_format twice, the first time
through the IMMEDIATE to get an appropriate string and the second time
to print that string. There's no more need for that. Therefore, calls to
IMMEDIATE are removed, and now we're directly printing the integer
values instead.
Signed-off-by: Milica Lazarevic
---
disas/nanomips.cpp | 756 -
1 file changed, 265 insertions(+), 491 deletions(-)
diff --git a/disas/nanomips.cpp b/disas/nanomips.cpp
index 816155527d..441204bb84 100644
--- a/disas/nanomips.cpp
+++ b/disas/nanomips.cpp
[...]
@@ -3305,11 +3271,9 @@ static char *CACHE(uint64 instruction, Dis_info *info)
uint64 rs_value = extract_rs_20_19_18_17_16(instruction);
int64 s_value = extract_s__se8_15_7_6_5_4_3_2_1_0(instruction);
-char *op = IMMEDIATE(op_value);
-char *s = IMMEDIATE(s_value);
const char *rs = GPR(rs_value);
-return img_format("CACHE %s, %s(%s)", op, s, rs);
+return img_format("CACHE 0x%" PRIx64 ", %s(%s)", op_value, s_value, rs);
}
@@ -3329,11 +3293,9 @@ static char *CACHEE(uint64 instruction, Dis_info *info)
uint64 rs_value = extract_rs_20_19_18_17_16(instruction);
int64 s_value = extract_s__se8_15_7_6_5_4_3_2_1_0(instruction);
-char *op = IMMEDIATE(op_value);
-char *s = IMMEDIATE(s_value);
const char *rs = GPR(rs_value);
-return img_format("CACHEE %s, %s(%s)", op, s, rs);
+return img_format("CACHEE 0x%" PRIx64 ", %s(%s)", op_value, s_value, rs);
}
Do we really want to format "int64 s_value" as a string? The code now
has lots of wrong format strings. Add the patch below to get the
compiler report.
We once had a discussion about using G_GNUC_PRINTF for local functions
or not. I think that this example clearly shows that it should be mandatory.
Regards,
Stefan
diff --git a/disas/nanomips.c b/disas/nanomips.c
index 9647f1a8e3..c875818cb9 100644
--- a/disas/nanomips.c
+++ b/disas/nanomips.c
@@ -95,7 +95,7 @@ typedef struct Pool {
#define IMGASSERTONCE(test)
-static char *img_format(const char *format, ...)
+static char * G_GNUC_PRINTF(1, 2) img_format(const char *format, ...)
{
char *buffer;
va_list args;
OpenPGP_0xE08C21D5677450AD.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v3 16/24] disas/nanomips: Remove IMMEDIATE functions
On 9/12/22 13:26, Milica Lazarevic wrote: Both versions of IMMEDIATE functions have been removed. Before this patch, we'd been calling img_format twice, the first time through the IMMEDIATE to get an appropriate string and the second time to print that string. There's no more need for that. Therefore, calls to IMMEDIATE are removed, and now we're directly printing the integer values instead. Signed-off-by: Milica Lazarevic --- disas/nanomips.cpp | 756 - 1 file changed, 265 insertions(+), 491 deletions(-) Reviewed-by: Richard Henderson r~
