[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 3/6] accel/tcg: Restrict tb_gen_code() from other accelerator
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH 3/6] accel/tcg: Restrict tb_gen_code() from other accelerators |
Date: |
Mon, 15 Mar 2021 15:48:41 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.0 |
On 3/15/21 2:52 PM, Claudio Fontana wrote:
> On 1/21/21 7:06 AM, Richard Henderson wrote:
>> On 1/17/21 11:12 PM, Claudio Fontana wrote:
>>> On 1/17/21 5:48 PM, Philippe Mathieu-Daudé wrote:
>>>> tb_gen_code() is only called within TCG accelerator,
>>>> declare it locally.
>>>
>>> Is this used only in accel/tcg/cpu-exec.c ? Should it be a static function
>>> there?
>>
>> Possibly, but there's a *lot* of code that would have to be moved. For now,
>> queuing a slightly modified version of the patch.
>>
>>>> --- a/accel/tcg/user-exec.c
>>>> +++ b/accel/tcg/user-exec.c
>>>> @@ -28,6 +28,7 @@
>>>> #include "qemu/atomic128.h"
>>>> #include "trace/trace-root.h"
>>>> #include "trace/mem.h"
>>>> +#include "internal.h"
>>
>> Not needed by this patch.
>>
>>
>> r~
>>
>
> Hello,
>
> resurrecting this, and in reference to its commit:
> "c03f041f128301c6a6c32242846be08719cd4fc3",
>
> the name "internal.h" ends up polluting the include paths,
> so that when working for example on s390x, including "internal.h" ends up
> including this instead of the file in target/s390x/.
>
> I am not sure what exactly the right solution is, for this specific problem,
> and if we should look at the include paths settings in detail,
>
> but in my view calling files just "internal.h" or "internals.h" in general is
> not a good idea.
>
> I can see two issues with this naming:
>
> 1) it describes nothing about the actual intended contents, other that they
> should be "internal".
> Rather it would be better to know what the file is intended to contain, or we
> end up (as we end up) with very large files containing completely unrelated
> content.
>
> 2) we end up with clashes in our include paths if we are not super careful.
>
> Probably in this case, the target/s390x/internal.h could be given another
> name (s390x-internal.h) and then split up in the future (there is a whole
> bunch of unrelated suff).
>
> For accel/tcg/internal.h, maybe renaming it, or removing it altogether could
> both be good options?
I think something like commit ed5bad39e57 is required, restricting
the scope of:
add_project_arguments('-iquote',
meson.current_source_dir() / 'tcg' / tcg_arch,
... to tcg, and ...
'-iquote',
meson.current_source_dir() / 'accel/tcg',
to accel.
language: ['c', 'cpp', 'objc'])