diff mbox series

[v3,1/5] dump: Include missing "cpu.h" header for tswap32/tswap64() declarations

Message ID 20221216215519.5522-2-philmd@linaro.org
State New
Headers show
Series target/cpu: System/User cleanups around hwaddr/vaddr | expand

Commit Message

Philippe Mathieu-Daudé Dec. 16, 2022, 9:55 p.m. UTC
Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 dump/dump.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Richard Henderson Dec. 16, 2022, 11:58 p.m. UTC | #1
On 12/16/22 13:55, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>   dump/dump.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index 279b07f09b..c62dc94213 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -29,6 +29,7 @@
>   #include "qemu/main-loop.h"
>   #include "hw/misc/vmcoreinfo.h"
>   #include "migration/blocker.h"
> +#include "cpu.h"

Does it work to include "exec/cpu-all.h" instead?


r~
Daniel Henrique Barboza Dec. 20, 2022, 10:44 a.m. UTC | #2
On 12/16/22 18:55, Philippe Mathieu-Daudé wrote:
> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---


Reviewed-by: Daniel Henrique Barboza <danielhb413@gmail.com>


>   dump/dump.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/dump/dump.c b/dump/dump.c
> index 279b07f09b..c62dc94213 100644
> --- a/dump/dump.c
> +++ b/dump/dump.c
> @@ -29,6 +29,7 @@
>   #include "qemu/main-loop.h"
>   #include "hw/misc/vmcoreinfo.h"
>   #include "migration/blocker.h"
> +#include "cpu.h"
>   
>   #ifdef TARGET_X86_64
>   #include "win_dump.h"
Philippe Mathieu-Daudé Feb. 23, 2023, 10:09 a.m. UTC | #3
On 17/12/22 00:58, Richard Henderson wrote:
> On 12/16/22 13:55, Philippe Mathieu-Daudé wrote:
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>> ---
>>   dump/dump.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/dump/dump.c b/dump/dump.c
>> index 279b07f09b..c62dc94213 100644
>> --- a/dump/dump.c
>> +++ b/dump/dump.c
>> @@ -29,6 +29,7 @@
>>   #include "qemu/main-loop.h"
>>   #include "hw/misc/vmcoreinfo.h"
>>   #include "migration/blocker.h"
>> +#include "cpu.h"
> 
> Does it work to include "exec/cpu-all.h" instead?

We get:

include/exec/cpu-all.h:110:5: warning: 'TARGET_LONG_SIZE' is not 
defined, evaluates to 0 [-Wundef]
#if TARGET_LONG_SIZE == 4
     ^

TARGET_LONG_SIZE is defined in "exec/cpu-defs.h" which is
target specific. If I add "exec/cpu-defs.h" to "exec/cpu-all.h"
I get:

In file included from ../../dump/dump.c:18:
include/exec/cpu-all.h:439:8: error: incomplete definition of type 
'struct ArchCPU'
     cpu->parent_obj.env_ptr = &cpu->env;
     ~~~^

Is it worth extracting the few tswapX() declarations to "exec/tswap.h"?
Richard Henderson Feb. 23, 2023, 6:01 p.m. UTC | #4
On 2/23/23 00:09, Philippe Mathieu-Daudé wrote:
>>> +#include "cpu.h"
>>
>> Does it work to include "exec/cpu-all.h" instead?
> 
> We get:
> 
> include/exec/cpu-all.h:110:5: warning: 'TARGET_LONG_SIZE' is not defined, evaluates to 0 
> [-Wundef]
> #if TARGET_LONG_SIZE == 4
>      ^
> 
> TARGET_LONG_SIZE is defined in "exec/cpu-defs.h" which is
> target specific. If I add "exec/cpu-defs.h" to "exec/cpu-all.h"
> I get:
> 
> In file included from ../../dump/dump.c:18:
> include/exec/cpu-all.h:439:8: error: incomplete definition of type 'struct ArchCPU'
>      cpu->parent_obj.env_ptr = &cpu->env;
>      ~~~^
> 
> Is it worth extracting the few tswapX() declarations to "exec/tswap.h"?

That's probably worthwhile, using cpu-param.h directly, perhaps, rather than pulling in 
the rest of cpu stuff?


r~
Philippe Mathieu-Daudé Feb. 23, 2023, 9:19 p.m. UTC | #5
On 23/2/23 19:01, Richard Henderson wrote:
> On 2/23/23 00:09, Philippe Mathieu-Daudé wrote:
>>>> +#include "cpu.h"
>>>
>>> Does it work to include "exec/cpu-all.h" instead?
>>
>> We get:
>>
>> include/exec/cpu-all.h:110:5: warning: 'TARGET_LONG_SIZE' is not 
>> defined, evaluates to 0 [-Wundef]
>> #if TARGET_LONG_SIZE == 4
>>      ^
>>
>> TARGET_LONG_SIZE is defined in "exec/cpu-defs.h" which is
>> target specific. If I add "exec/cpu-defs.h" to "exec/cpu-all.h"
>> I get:
>>
>> In file included from ../../dump/dump.c:18:
>> include/exec/cpu-all.h:439:8: error: incomplete definition of type 
>> 'struct ArchCPU'
>>      cpu->parent_obj.env_ptr = &cpu->env;
>>      ~~~^
>>
>> Is it worth extracting the few tswapX() declarations to "exec/tswap.h"?
> 
> That's probably worthwhile, using cpu-param.h directly, perhaps, rather 
> than pulling in the rest of cpu stuff?

TARGET_BIG_ENDIAN is only defined for target-specific objects, so this
as is it will never work (dump.o must be compiled for each target).

For heterogeneous emulation we need to pass a CPU[Arch]State* to
get the endianness of the CPU. As of today the endianness isn't
runtime, so this field doesn't exist in the common CPUState (some
arch might have some equivalent field).


This file uses tswap() 4 times in the same function: get_note_sizes(),
so I could extract it to a dump-target.c unit.
I have no clue what that file is for, but this particularity is odd.

Looking further, we have in "hw/core/cpu.h":

   int (SysemuCPUOps::write_elf32/64_note)(WriteCoreDumpFunction, ...

but no ReadCoreDumpFunction equivalent.
Richard Henderson Feb. 23, 2023, 9:43 p.m. UTC | #6
On 2/23/23 11:19, Philippe Mathieu-Daudé wrote:
> This file uses tswap() 4 times in the same function: get_note_sizes(),
> so I could extract it to a dump-target.c unit.
> I have no clue what that file is for, but this particularity is odd.

All uses of tswap in that file are wrong, and should be using cpu_to_dumpN, which 
correctly tests the endianness of the output.


r~
Philippe Mathieu-Daudé Feb. 23, 2023, 10:29 p.m. UTC | #7
On 23/2/23 22:43, Richard Henderson wrote:
> On 2/23/23 11:19, Philippe Mathieu-Daudé wrote:
>> This file uses tswap() 4 times in the same function: get_note_sizes(),
>> so I could extract it to a dump-target.c unit.
>> I have no clue what that file is for, but this particularity is odd.
> 
> All uses of tswap in that file are wrong, and should be using 
> cpu_to_dumpN, which correctly tests the endianness of the output.

Yes! Thank you :)
diff mbox series

Patch

diff --git a/dump/dump.c b/dump/dump.c
index 279b07f09b..c62dc94213 100644
--- a/dump/dump.c
+++ b/dump/dump.c
@@ -29,6 +29,7 @@ 
 #include "qemu/main-loop.h"
 #include "hw/misc/vmcoreinfo.h"
 #include "migration/blocker.h"
+#include "cpu.h"
 
 #ifdef TARGET_X86_64
 #include "win_dump.h"