Andres Freund <andres@xxxxxxxxxxx> 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
