Re: [RFC][PATCH 03/36] objtool: Enable compilation of objtool for all architectures

[Julien Thierry <jthierry@redhat.com>]

Hi Matt,

On 4/14/20 9:56 PM, Matt Helsley wrote:
> On Tue, Apr 14, 2020 at 08:39:23AM +0100, Julien Thierry wrote:
>> So, if it is decided that recordmcount should be an objtool subcommand, the
>> code itself should probably stay under tools/objtool and then have different
>> compilation configurations for objtool depending on the architecture (e.g.
>> HAVE_OBJTOOL_CHECK, HAVE_OBJTOOL_ORC) or something of the sort.
> 
> Yeah. HAVE_C_RECORDMCOUNT is used in the Makefiles to select building
> an running objtool mcount versus recordmcount.pl (which is another piece
> that needs some attention -- my preference is to slowly move arch
> support from there into recordmcount.c. So far it looks like s390 and mips
> are the ones needing a little special attention there..)
> 
> My recollection is Josh wanted to avoid a bunch of architecture/config
> checks in the code if I recall. It leaks into the code that implements the
> subcommand tables, for example, and that's why I chose to use weak symbols --
> we can unconditionally add the table entries and we don't need to play
> linker script + macro games to build the array.
> 
> As for managing minor architectural variations in a single subcommand, either
> those can use more weak symbols via arch/foo/subcmd.[ch] files or via explicit
> checks in the code (see the arch, endian, and class switches in recordmcount.c
> for the latter). If folks are OK with using weak symbols I'm curious what
> preferences are on choosing when to use which method -- the RFC reflects
> mine of course but I want to know what makes it more maintainable for
> other folks.
> 

Thanks for providing the background reasoning.

I think that using weak symbols instead of macros to conditionally 
compile is fine. However, I still think that temporarily moving code 
that could be shared across architectures once the necessary back-end is 
implemented is not the right way. For instance, in the case of check(), 
it should be arch specific parts it relies on that should be weak 
symbols (e.g. arch_decode_instruction()).

And in order to have a correct list of supported subcommands, maybe that 
could be done in arch specific back end (e.g. arch_get_subcommand_set() 
), which would fill the array of subcommands for the target 
architecture. And you could have a default "weak" implementation that 
fills the array with subcommands that do not rely on any arch specific 
support.

This way we don't introduce #ifdef into the code and we don't move the 
generic code.

Might not be the prettiest option, but would it be a good enough 
compromise? Or are there other suggestions?

Thanks,

-- 
Julien Thierry