Re: [Qemu-devel] [PATCH v1 4/4] elf: move ELF_ARCH definition to elf-arch.h

[Alex Bennée]

Richard Henderson <address@hidden> writes:

> On 9/11/19 5:26 AM, Alex Bennée wrote:
>>
>> Aleksandar Markovic <address@hidden> writes:
>>
>>> 10.09.2019. 21.34, "Alex Bennée" <address@hidden> је написао/ла:
>>>>
>>>> This is preparatory for plugins which will want to report the
>>>> architecture to plugins. Move the ELF_ARCH definition out of the
>>>> loader and into its own header.
>>>>
>>>> Signed-off-by: Alex Bennée <address@hidden>
>>>> --
>>>
>>> Hi, Alex.
>>>
>>> I advice some caution here
>>> . For example, EM_NANOMIPS, and some other EM_xxx constants are not
>>> included in the new header.
>>
>> EM_ARCH was never set to EM_NANOMIPS. In fact I can see e_machine is
>> checked against it so I guess it is only ever used to checking the
>> loading elf directly.
>>
>> In fact EM_ARCH is only really the default fallback case for checking
>> the elf type if there is not a more "forgiving" check done by arch
>> specific code (elf_check_arch). The only other cace is the fallback for
>> EM_MACHINE unless PPC does something with it.
>>
>>> I am not sure what exactly you want to achieve
>>> with this refactoring. But you may miss your goal, since some EM_xxx will
>>> be missing from your new header. Is this the right way to achieve what you
>>> want, and could you unintentionally introduce bugs? Can you outline in more
>>> details your intentions around the new header? Is this refactoring the only
>>> way?
>>
>> As mentioned in the other reply maybe exposing a value from configure
>> into config-target.[mak|h] would be a better approach?
>
> I think using EM_FOO to tell a plugin about the architecture is just going to
> be confusing.  As seen here wrt EM_NANOMIPS, but arguably elsewhere with
> EM_SPARC vs EM_SPARC64.
>
> You need to decide what it is that you want the plugin to know.  It is just be
> gross architecture?  In which case QEMU_ARCH_FOO is probably enough.  Is it 
> the
> instruction set currently executing?  In which case neither EM_FOO nor
> QEMU_ARCH_FOO is sufficient -- e.g. arm thumb -- and you'll have to invent
> something new, because no two architectures handle this in the same way.  The
> closest you might be able to get without invention from whole cloth is the
> capstone cap_arch+cap_mode values from cc->disas_set_info().  But that only
> handles the most popular of targets.

I think I'm going to stick with the gross TARGET for now. It mostly
seems a way of avoiding building per-arch plugins. I assume most out of
tree plugins will be targeted at a specific machine anyway.

Anyway I think that means I'll drop this series unless 1-3 are worth
keeping as simple clean-ups leaving out 4?

>
> In any case, a single header that every target must touch is the wrong
> approach.  If you want to do some cleanup, move these defines into e.g.
> linux-user/$TARGET/target_elf.h.  (And I wouldn't bother touching bsd-user in
> its current condition.)
>
>
> r~


--
Alex Bennée