2022-07-03 21:26:53

by Andres Freund

[permalink] [raw]
Subject: [PATCH v2 2/5] tools include: add dis-asm-compat.h to handle version differences

binutils changed the signature of init_disassemble_info(), which now causes
compilation failures for tools/{perf,bpf}, e.g. on debian unstable.
Relevant binutils commit:
https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07

This commit introduces a wrapper for init_disassemble_info(), to avoid
spreading #ifdef DISASM_INIT_STYLED to a bunch of places. Subsequent
commits will use it to fix the build failures.

It likely is worth adding a wrapper for disassember(), to avoid the already
existing DISASM_FOUR_ARGS_SIGNATURE ifdefery.

Cc: Alexei Starovoitov <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Sedat Dilek <[email protected]>
Cc: Quentin Monnet <[email protected]>
Link: http://lore.kernel.org/lkml/[email protected]
Signed-off-by: Andres Freund <[email protected]>
---
tools/include/tools/dis-asm-compat.h | 53 ++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)
create mode 100644 tools/include/tools/dis-asm-compat.h

diff --git a/tools/include/tools/dis-asm-compat.h b/tools/include/tools/dis-asm-compat.h
new file mode 100644
index 000000000000..d1d003ee3e2f
--- /dev/null
+++ b/tools/include/tools/dis-asm-compat.h
@@ -0,0 +1,53 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef _TOOLS_DIS_ASM_COMPAT_H
+#define _TOOLS_DIS_ASM_COMPAT_H
+
+#include <stdio.h>
+#include <linux/compiler.h>
+#include <dis-asm.h>
+
+/* define types for older binutils version, to centralize ifdef'ery a bit */
+#ifndef DISASM_INIT_STYLED
+enum disassembler_style {DISASSEMBLER_STYLE_NOT_EMPTY};
+typedef int (*fprintf_styled_ftype) (void *, enum disassembler_style, const char*, ...);
+#endif
+
+/*
+ * Trivial fprintf wrapper to be used as the fprintf_styled_func argument to
+ * init_disassemble_info_compat() when normal fprintf suffices.
+ */
+static inline int fprintf_styled(void *out,
+ enum disassembler_style style __maybe_unused,
+ const char *fmt, ...)
+{
+ va_list args;
+ int r;
+
+ va_start(args, fmt);
+ r = vfprintf(out, fmt, args);
+ va_end(args);
+
+ return r;
+}
+
+/*
+ * Wrapper for init_disassemble_info() that hides version
+ * differences. Depending on binutils version and architecture either
+ * fprintf_func or fprintf_styled_func will be called.
+ */
+static inline void init_disassemble_info_compat(struct disassemble_info *info,
+ void *stream,
+ fprintf_ftype unstyled_func,
+ fprintf_styled_ftype styled_func __maybe_unused)
+{
+#ifdef DISASM_INIT_STYLED
+ init_disassemble_info(info, stream,
+ unstyled_func,
+ styled_func);
+#else
+ init_disassemble_info(info, stream,
+ unstyled_func);
+#endif
+}
+
+#endif /* _TOOLS_DIS_ASM_COMPAT_H */
--
2.37.0.3.g30cc8d0f14


2022-07-05 14:31:03

by Quentin Monnet

[permalink] [raw]
Subject: Re: [PATCH v2 2/5] tools include: add dis-asm-compat.h to handle version differences

On 03/07/2022 22:25, Andres Freund wrote:
> binutils changed the signature of init_disassemble_info(), which now causes
> compilation failures for tools/{perf,bpf}, e.g. on debian unstable.
> Relevant binutils commit:
> https://sourceware.org/git/?p=binutils-gdb.git;a=commit;h=60a3da00bd5407f07
>
> This commit introduces a wrapper for init_disassemble_info(), to avoid
> spreading #ifdef DISASM_INIT_STYLED to a bunch of places. Subsequent
> commits will use it to fix the build failures.
>
> It likely is worth adding a wrapper for disassember(), to avoid the already
> existing DISASM_FOUR_ARGS_SIGNATURE ifdefery.
>
> Cc: Alexei Starovoitov <[email protected]>
> Cc: Arnaldo Carvalho de Melo <[email protected]>
> Cc: Sedat Dilek <[email protected]>
> Cc: Quentin Monnet <[email protected]>
> Link: http://lore.kernel.org/lkml/[email protected]
> Signed-off-by: Andres Freund <[email protected]>
> ---
> tools/include/tools/dis-asm-compat.h | 53 ++++++++++++++++++++++++++++
> 1 file changed, 53 insertions(+)
> create mode 100644 tools/include/tools/dis-asm-compat.h
>
> diff --git a/tools/include/tools/dis-asm-compat.h b/tools/include/tools/dis-asm-compat.h
> new file mode 100644
> index 000000000000..d1d003ee3e2f
> --- /dev/null
> +++ b/tools/include/tools/dis-asm-compat.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

Any chance you could contribute this wrapper as dual-licenced
(GPL-2.0-only OR BSD-2-Clause), for better compatibility with the rest
of bpftool's code?

The rest of the set looks good to me. Thanks a lot for this work!
Quentin

2022-07-15 19:45:52

by Andres Freund

[permalink] [raw]
Subject: Re: [PATCH v2 2/5] tools include: add dis-asm-compat.h to handle version differences

Hi,

On 2022-07-05 14:44:07 +0100, Quentin Monnet wrote:
> > diff --git a/tools/include/tools/dis-asm-compat.h b/tools/include/tools/dis-asm-compat.h
> > new file mode 100644
> > index 000000000000..d1d003ee3e2f
> > --- /dev/null
> > +++ b/tools/include/tools/dis-asm-compat.h
> > @@ -0,0 +1,53 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
>
> Any chance you could contribute this wrapper as dual-licenced
> (GPL-2.0-only OR BSD-2-Clause), for better compatibility with the rest
> of bpftool's code?

Happy to do that from my end - however, right now it includes
linux/compiler.h, which is GPL-2.0. I don't know what the policy around that
is - is it just a statement about the licence of the header itself, or does it
effectively include its dependencies?

FWIW, linux/compiler.h is also included from bpftool.

If preferrable, I can replace the linux/compiler.h include by just using
__attribute__((__unused__)) directly or by using a (void) cast to avoid the
unused-parameter pedantry.

Greetings,

Andres Freund

2022-07-15 20:00:24

by Andres Freund

[permalink] [raw]
Subject: Re: [PATCH v2 2/5] tools include: add dis-asm-compat.h to handle version differences

Hi,

On 2022-07-15 12:39:27 -0700, Andres Freund wrote:
> On 2022-07-05 14:44:07 +0100, Quentin Monnet wrote:
> > > diff --git a/tools/include/tools/dis-asm-compat.h b/tools/include/tools/dis-asm-compat.h
> > > new file mode 100644
> > > index 000000000000..d1d003ee3e2f
> > > --- /dev/null
> > > +++ b/tools/include/tools/dis-asm-compat.h
> > > @@ -0,0 +1,53 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> >
> > Any chance you could contribute this wrapper as dual-licenced
> > (GPL-2.0-only OR BSD-2-Clause), for better compatibility with the rest
> > of bpftool's code?
>
> Happy to do that from my end - however, right now it includes
> linux/compiler.h, which is GPL-2.0. I don't know what the policy around that
> is - is it just a statement about the licence of the header itself, or does it
> effectively include its dependencies?

FWIW, dis-asm.h itself is GPL-3-or-later.

Greetings,

Andres Freund

2022-07-18 09:19:53

by Quentin Monnet

[permalink] [raw]
Subject: Re: [PATCH v2 2/5] tools include: add dis-asm-compat.h to handle version differences

On 15/07/2022 20:39, Andres Freund wrote:
> Hi,
>
> On 2022-07-05 14:44:07 +0100, Quentin Monnet wrote:
>>> diff --git a/tools/include/tools/dis-asm-compat.h b/tools/include/tools/dis-asm-compat.h
>>> new file mode 100644
>>> index 000000000000..d1d003ee3e2f
>>> --- /dev/null
>>> +++ b/tools/include/tools/dis-asm-compat.h
>>> @@ -0,0 +1,53 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>
>> Any chance you could contribute this wrapper as dual-licenced
>> (GPL-2.0-only OR BSD-2-Clause), for better compatibility with the rest
>> of bpftool's code?
>
> Happy to do that from my end - however, right now it includes
> linux/compiler.h, which is GPL-2.0. I don't know what the policy around that
> is - is it just a statement about the licence of the header itself, or does it
> effectively include its dependencies?

My understanding is that programs using a GPL header need to be released
as GPL, but I don't believe they have to be only GPL, the dual-license
should cover the requirements. If someone wanted to redistribute the
code from the new header dis-asm-compat.h as BSD only, they would
probably have to get rid of the GPL-only dependencies though. But again,
this is only my understanding, and “I am not a lawyer”.

>
> FWIW, linux/compiler.h is also included from bpftool.
>
> If preferrable, I can replace the linux/compiler.h include by just using
> __attribute__((__unused__)) directly or by using a (void) cast to avoid the
> unused-parameter pedantry.

If compiler.h is just needed for the “unused” attribute, I wouldn't mind
doing that.

Thanks,
Quentin