[PATCH] Add room for insn-enconding and symbol name

[PATCH] Add room for insn-enconding and symbol name

[Sergio González Collado]

The longest length of a symbol (KSYM_NAME_LEN) was increased to 512
in the reference [1]. This patch adds room for insn-encoding and a
symbol name, as proposed in [2]

[1] https://lore.kernel.org/lkml/20220802015052.10452-6-ojeda@kernel.org/
[2] https://lore.kernel.org/lkml/16641a56-9139-4396-a5a8-89606bedd1f1@app.fastmail.com/

Signed-off-by: Sergio González Collado <sergio.collado@gmail.com>
---
 arch/x86/tools/insn_decoder_test.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/tools/insn_decoder_test.c b/arch/x86/tools/insn_decoder_test.c
index 472540aeabc2..add51bfc2260 100644
--- a/arch/x86/tools/insn_decoder_test.c
+++ b/arch/x86/tools/insn_decoder_test.c
@@ -10,6 +10,7 @@
 #include <assert.h>
 #include <unistd.h>
 #include <stdarg.h>
+#include <linux/kallsym.h>
 
 #define unlikely(cond) (cond)
 
@@ -106,7 +107,7 @@ static void parse_args(int argc, char **argv)
 	}
 }
 
-#define BUFSIZE 256
+#define BUFSIZE (256 + KSYM_NAME_LEN)
 
 int main(int argc, char **argv)
 {
-- 
2.39.2

Re: [PATCH] Add room for insn-enconding and symbol name

[Danilo Krummrich]

On 4/14/24 20:09, Sergio González Collado wrote:
> The longest length of a symbol (KSYM_NAME_LEN) was increased to 512
> in the reference [1]. This patch adds room for insn-encoding and a
> symbol name, as proposed in [2]
> 
> [1] https://lore.kernel.org/lkml/20220802015052.10452-6-ojeda@kernel.org/
> [2] https://lore.kernel.org/lkml/16641a56-9139-4396-a5a8-89606bedd1f1@app.fastmail.com/
> 
> Signed-off-by: Sergio González Collado <sergio.collado@gmail.com>

Acked-by: Danilo Krummrich <dakr@redhat.com>

> ---
>   arch/x86/tools/insn_decoder_test.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/tools/insn_decoder_test.c b/arch/x86/tools/insn_decoder_test.c
> index 472540aeabc2..add51bfc2260 100644
> --- a/arch/x86/tools/insn_decoder_test.c
> +++ b/arch/x86/tools/insn_decoder_test.c
> @@ -10,6 +10,7 @@
>   #include <assert.h>
>   #include <unistd.h>
>   #include <stdarg.h>
> +#include <linux/kallsym.h>
>   
>   #define unlikely(cond) (cond)
>   
> @@ -106,7 +107,7 @@ static void parse_args(int argc, char **argv)
>   	}
>   }
>   
> -#define BUFSIZE 256
> +#define BUFSIZE (256 + KSYM_NAME_LEN)
>   
>   int main(int argc, char **argv)
>   {

Re: [PATCH] Add room for insn-enconding and symbol name

[Danilo Krummrich]

Hi,

On 5/8/24 13:58, Danilo Krummrich wrote:
> On 4/14/24 20:09, Sergio González Collado wrote:
>> The longest length of a symbol (KSYM_NAME_LEN) was increased to 512
>> in the reference [1]. This patch adds room for insn-encoding and a
>> symbol name, as proposed in [2]
>>
>> [1] https://lore.kernel.org/lkml/20220802015052.10452-6-ojeda@kernel.org/
>> [2] https://lore.kernel.org/lkml/16641a56-9139-4396-a5a8-89606bedd1f1@app.fastmail.com/
>>
>> Signed-off-by: Sergio González Collado <sergio.collado@gmail.com>

What's the status on this one? Are there any concerns or can we land this?

- Danilo

> 
> Acked-by: Danilo Krummrich <dakr@redhat.com>
> 
>> ---
>>   arch/x86/tools/insn_decoder_test.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/tools/insn_decoder_test.c b/arch/x86/tools/insn_decoder_test.c
>> index 472540aeabc2..add51bfc2260 100644
>> --- a/arch/x86/tools/insn_decoder_test.c
>> +++ b/arch/x86/tools/insn_decoder_test.c
>> @@ -10,6 +10,7 @@
>>   #include <assert.h>
>>   #include <unistd.h>
>>   #include <stdarg.h>
>> +#include <linux/kallsym.h>
>>   #define unlikely(cond) (cond)
>> @@ -106,7 +107,7 @@ static void parse_args(int argc, char **argv)
>>       }
>>   }
>> -#define BUFSIZE 256
>> +#define BUFSIZE (256 + KSYM_NAME_LEN)
>>   int main(int argc, char **argv)
>>   {

Re: [PATCH] Add room for insn-enconding and symbol name

[Sergio González Collado]

I don't have any news on my side.

Re: [PATCH] Add room for insn-enconding and symbol name

[Thomas Gleixner]

Sergio!

On Sun, Apr 14 2024 at 20:09, Sergio González Collado wrote:

Please read through Documentation/process/maintainers-tip.rst. It
explains details about how change logs and patch subjects should be
written.

> The longest length of a symbol (KSYM_NAME_LEN) was increased to 512
> in the reference [1]. This patch adds room for insn-encoding and a
> symbol name, as proposed in [2]

This is really not a good explanation. The changelog wants to be self
contained and explain the problem to be solved. Having to follow random
archive links to get an explanation is just painful. The links can only
serve as additional material.

> [1] https://lore.kernel.org/lkml/20220802015052.10452-6-ojeda@kernel.org/
> [2] https://lore.kernel.org/lkml/16641a56-9139-4396-a5a8-89606bedd1f1@app.fastmail.com/
>
> Signed-off-by: Sergio González Collado <sergio.collado@gmail.com>
> ---
>  arch/x86/tools/insn_decoder_test.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/tools/insn_decoder_test.c b/arch/x86/tools/insn_decoder_test.c
> index 472540aeabc2..add51bfc2260 100644
> --- a/arch/x86/tools/insn_decoder_test.c
> +++ b/arch/x86/tools/insn_decoder_test.c
> @@ -10,6 +10,7 @@
>  #include <assert.h>
>  #include <unistd.h>
>  #include <stdarg.h>
> +#include <linux/kallsym.h>

Please put a newline between the existing includes and the linux
prefixed one.

>  #define unlikely(cond) (cond)
>  
> @@ -106,7 +107,7 @@ static void parse_args(int argc, char **argv)
>  	}
>  }
>  
> -#define BUFSIZE 256
> +#define BUFSIZE (256 + KSYM_NAME_LEN)

What's the rationale here?

256 + KSYM_NAME_LEN is as arbitrary as defining the buffer size to
4096, i.e. it's a number pulled out of thin air.

If you really want to make this code resilent against future line length
changes then the obvious thing to do is:

static char *update_buf(char *buf, ssize_t size)
{
	buf = realloc(buf, size);
        if (!buf) {
        	complain();
        	exit(9);
	}
}

main()
{
	char *buffer = NULL, *sym = NULL, *copy = NULL;
	ssize_t size = 256, oldsize = 0, len;

	for (len = getline(&buffer, &size, stdin); len > 0;
             len = getline(&buffer, &size, stdin) {
        	if (size != oldsize) {
              		update_buf(sym, size);
			update_buf(copy, size);
		}
                buffer[len - 1] = '\0';
                ...
        }
        if (errno)
        	complain(errno);

	free(buffer);
        free(sym);
        free(copy);
        ...

No?

Thanks,

        tglx