Message-ID: <871qvfd800.fsf@redhat.com>
Date:   Thu, 23 Jun 2022 10:49:51 +0100
From:   Andrew Burgess <aburgess@...hat.com>
To:     Andres Freund <andres@...razel.de>,
        Quentin Monnet <quentin@...valent.com>
Cc:     open list <linux-kernel@...r.kernel.org>,
        bpf <bpf@...r.kernel.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>
Subject: Re: init_disassemble_info() signature changes causes compile failures

Andres Freund <andres@...razel.de> writes:

> Hi,
>
> On 2022-06-22 23:53:58 +0100, Quentin Monnet wrote:
>> Too bad the libbfd API is changing again :/
>
> Yea, not great. Particularly odd that
> /* For compatibility with existing code.  */
> #define INIT_DISASSEMBLE_INFO(INFO, STREAM, FPRINTF_FUNC, FPRINTF_STYLED_FUNC)  \
>
> was changed. Leaving the "For compatibility with existing code." around,
> despite obviously not providing compatibility...
>
> CCed the author of that commit, maybe worth fixing?

Greetings!

First, massive appologies for breaking you existing code.  I wasn't
aware that the libopcodes disassembler was used by anything out of
tree.

> Given that disassemble_set_printf() was added, it seems like it'd have been
> easy to not change init_disassemble_info() / INIT_DISASSEMBLE_INFO() and
> require disassemble_set_printf() to be called to get styled printf support.

When I did this work I desperately wanted to maintain the original API
as much as possible.  But I couldn't figure out a way for this to work.

The problem is that we now have two classes of disassembler, those that
call the styled printf callback, and those that call the classic
non-styled printf.

When I originally did this work I did want to leave
INIT_DISASSEMBLE_INFO untouched, and provide a default styled printf
that would forward calls to the non-styled printf.

The problem I ran into is that the disassemble_info printf API is not
great.  If the API had passed the disassemble_info* as one of the
arguments then this would have been trivial.  But instead, we only get
passed a 'void *', which is one of the fields of disassemble_info.

As a result there's no easy way to forward calls from this imagined
default styled printf, to the actual, user supplied non-styled printf.

I did consider changing the 'void *' field from being the stream to
write to, to be the disassemble_info*, but then the non-styled printf
calls would need to be updated anyway, which would be a breaking change.

I also considered changing the 'void *' stream to be the
'disassemble_info*', then wrapping both styled and non-styled printf
calls.  This would allow me to provide a default for the styled printf,
and we can pull the required information from the 'disassemble_info*',
but the problem I have now is that the wrapper functions would be a
vararg function, and the user supplied printf functions are also vararg
functions.... and I didn't know how to forward the args from my wrapper
to the user supplied functions without changing the API of the user
supplied functions to take a va_list ... which again is an API breaking
change.

I'm aware that non of the above helps you at all, other than to say, I
didn't make this change without a little thought.  But, that doesn't
mean there isn't a better way this could have been done.  So, if you
have any suggestions, then I'm happy to give them a go.

Once again, sorry for the additional work this has created for you, and
if I can help at all to put this right, then please, do let me know.

Oh, and you're absolutely correct about the comment.  I should have just
deleted it.  Or really, I should have just deleted the macro entirely I
guess and forced everyone to call init_disassemble_info directly.  Bit
late for that now though!

Thanks,
Andrew


>> > The fix is easy enough, add a wrapper around fprintf() that conforms to the
>> > new signature.
>> >
>> > However I assume the necessary feature test and wrapper should only be added
>> > once? I don't know the kernel stuff well enough to choose the right structure
>> > here.
>> 
>> We can probably find a common header for the wrapper under
>> tools/include/. One possibility could be a new header under
>> tools/include/tools/, like for libc_compat.h. Although personally I
>> don't mind too much about redefining the wrapper several times given
>> how short it is, and because maybe some tools could redefine it anyway
>> to use colour output in the future.
>
> I'm more bothered by duplicating the necessary ifdefery than duplicating the
> short fprintf wrapper...
>
>
>> The feature test would better be shared, it would probably be similar
>> to what was done in the following commit to accommodate for a previous
>> change in libbfd:
>> 
>>     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=fb982666e380c1632a74495b68b3c33a66e76430
>
> Ah, beautiful hand-rolled feature tests :)
>
>
>> > Attached is my local fix for perf. Obviously would need work to be a real
>> > solution.
>> 
>> Thanks a lot! Would you be willing to submit a patch for the feature
>> detection and wrapper?
>
> I'll give it a go, albeit probably not today.
>
> Greetings,
>
> Andres Freund

Powered by blists - more mailing lists