instrument functions: Satisfy bfd inclusion guard

[corubba]

In 2012 binutils added an inclusion guard to bfd.h, requiring to include the automake config.h beforehand. This guard is triggered when enabling instrumentation and development mode:

% git clone https://github.com/the-tcpdump-group/tcpdump.git
% cd tcpdump
% touch .devel
% ./autogen.sh
% ./configure --enable-instrument-functions --enable-smb --quiet
./mkdep -c gcc -m -M -s . -DHAVE_CONFIG_H -I. fptype.c tcpdump.c print-smb.c smbutil.c instrument-functions.c <libnetdissect src list>
In file included from ./instrument-functions.c:18:
/usr/include/bfd.h:35:2: error: #error config.h must be included before this header
   35 | #error config.h must be included before this header
      |  ^~~~~

Note than this does *not* happen on Debian-based systems, because they remove that guard since 2012. That's also why the Ubuntu-based Linux CI is not affected. But on distributions using the vanilla source, like Arch Linux, it prevents the debug build.

Also a small update to the CONTRIBUTING docs, adding the required automake autoreconf invocation.

[fxlb]

"adding the required automake command" ?

[corubba]

"adding the required automake command" ?

My bad, that should be autoconf.

[fxlb]

In fact : autogen.sh

[fxlb]

It's not just autoreconf.

[infrastation]

So, the bfd.h expects the project that uses binutils to include the project's own config.h, not the config.h from binutils, correct? If that is the case, it would help to have a short comment in instrument-functions.c to remind about the purpose of the new #include.

Regarding "Ubuntu-based Linux CI", currently only the Coverity branch (which indeed uses Ubuntu) enables the instrument functions, regular builds do not do regardless of the OS and distribution. To that end, linux-aarch64 runs on Alpine Linux if that provides an easy way to reproduce the problem. Alternatively, it would not be difficult to reinstall linux-amd64 using Fedora, CentOS, AlmaLinux or Rocky Linux. The potential instrument functions test would likely need to be an off-by-default step with an environment variable such as TEST_INSTRUMENT=yes allowing to enable it for selected worker hosts.

[guyharris]

Also a small update to the CONTRIBUTING docs, adding the required automake autoreconf invocation.

The two changes here are separate; the CONTRIBUTING.md change should probably be a separate pull request.

[guyharris]

The two changes here are separate; the CONTRIBUTING.md change should probably be a separate pull request.

Not any more; I've changed CONTRIBUTING.md to have more steps, including an autogen.sh step.

[guyharris]

In 2012 binutils added an inclusion guard to bfd.h, requiring to include the automake config.h beforehand.

Given that bfd.h is, as far as I can tell, a public header file, exported for use by programs that use libbfd, why on earth would they do that? There is no guarantee, for example, that all code using libbfd even has a config.h file.

Note than this does not happen on Debian-based systems, because they remove that guard since 2012.

I don't know why they did that - or why they didn't push back on the binutils maintainers, if, as I suspect is the case, they found that it caused problems to users of binutils (as opposed to developersof binutils).

[fxlb]

Given that bfd.h is, as far as I can tell, a public header file, exported for use by programs that use libbfd, why on earth would they do that? There is no guarantee, for example, that all code using libbfd even has a config.h file.

+1

[corubba]

My reading of the original binutils PR 14072 (and its follow-ups 14243 and 15920) is that the inclusion guard is designed as some binutils-internal thing, and they didn't/don't expect other projects to directly use/include it. And those who are will need to work around the guard by supply any config.h (or something that behaves like one) to stop it from complaining, even if that project isn't actually using autoconf.

I deemed it best to just include the existing config.h since tcpdump is using autoconf, even if the contained macros probably won't change anything apart from satisfying that inclusion guard. Alternatively you could instead just (re-)define the two checked-for-by-binutils macros with bogus-values, like suggested in this stackoverflow answer.

Rebased, dropped the now-superseeded docs change, and added a comment to the include. Not sure why the netbsd-mips64 ci-build failed, or how to investigate/fix that.

[fxlb]

I'm not sure anything needs to be changed. It's more up to the distributions maintainers to fix the problem in bfd.h with the package maintainers, or to apply the same fix as Debian.

[guyharris]

My reading of the original binutils PR 14072 (and its follow-ups 14243 and 15920) is that the inclusion guard is designed as some binutils-internal thing, and they didn't/don't expect other projects to directly use/include it.''

The "How To Use BFD" section of the libbfd documentation says "To use the library, include bfd.h and link with libbfd.a." I don't know who first made an exported library out of it - the binutils developers, one or more random Linux distributions, or somebody else, but whoever it was should have fixed that, either by making bfd.h a usable external header (moving that guard somewhere else), making a new suitable header for use by arbitrary programs using libbfd, or documenting that annoying requirement.

I deemed it best to just include the existing config.h since tcpdump is using autoconf,

Or, rather, that it can use either autoconf or CMake, and both of them, as used in tcpdump, define a config.h file that defines the PACKAGE_VERSION macro (neither of them define PACKAGE).

Not sure why the netbsd-mips64 ci-build failed,

One reason appears to be "the builder for it sometimes gets memory errors or something such as that, causing it to randomly crash".

[fxlb]

No include.
Eventually #define PACKAGE "workaround-bfd.h-is-sometimes-buggy"
With a comment.
(PACKAGE has no other use in our code. This is why we could use it here.)

[fxlb]

Tested on Arch Linux with docker run -it archlinux.

[corubba]

No include.

Would you mind sharing your reason(ing) for that opinion/decision? I am just trying to understand and learn.

Lets forget for a moment that silly inclusion guard, and instead imagine that the bfd.h does some clever optimizations based on autoconf-style macros when certain optional headers/libraries are present. In that case you probably wouldn't set the required macros manually, but just include config.h at the top of the file like it is done already in 96% of all *.c files in this repository (I actually checked that); and possibly add some more detections in autoconf.
What I am trying to say is: All emotions aside, is this really so special that it requires a unique solution instead of just using the standard-solution that's already widely used in this here codebase?

Yes, that inclusion guard is questionable, and it sucks having to work around it downstream. But the objections back in 2012 were rejected, and by now I assume it's so accepted and entrenched that upstream will all the more not change it. You can pass the buck to distributions and package maintainers, but that won't help in the (maybe theoretical) scenario when building everything directly from upstream-source.

[fxlb]

No include.

Would you mind sharing your reason(ing) for that opinion/decision? I am just trying to understand and learn.

Lets forget for a moment that silly inclusion guard, and instead imagine that the bfd.h does some clever optimizations based on autoconf-style macros when certain optional headers/libraries are present. In that case you probably wouldn't set the required macros manually, but just include config.h at the top of the file like it is done already in 96% of all *.c files in this repository (I actually checked that);

As Guy had pointed out:
"1.2 How To Use BFD

To use the library, include bfd.h and link with libbfd.a. "

There is nothing about "config.h" or "optimizations" in the documentation (link above).

Thus, there is no reason to add unused macros.

"silly inclusion" -> "workround hack" (like the Debian one).

and possibly add some more detections in autoconf. What I am trying to say is: All emotions aside, is this really so special that it requires a unique solution instead of just using the standard-solution that's already widely used in this here codebase?

Yes, that inclusion guard is questionable, and it sucks having to work around it downstream. But the objections back in 2012 were rejected,

Any links about the reasons of the reject?

and by now I assume it's so accepted and entrenched that upstream will all the more not change it. You can pass the buck to distributions and package maintainers, but that won't help in the (maybe theoretical) scenario when building everything directly from upstream-source.

Debian maintainers have done the work, Others could do the same...

[corubba]

Any links about the reasons of the reject?

The ones I found are already posted above, especially the two follow-ups issues:

My reading of the original binutils PR 14072 (and its follow-ups 14243 and 15920) is that the inclusion guard is designed as some binutils-internal thing, and they didn't/don't expect other projects to directly use/include it. And those who are will need to work around the guard by supply any config.h (or something that behaves like one) to stop it from complaining, even if that project isn't actually using autoconf.

[guyharris]

Any links about the reasons of the reject?

The ones I found are already posted above, especially the two follow-ups issues:

My reading of the original binutils PR 14072 (and its follow-ups 14243 and 15920) is that the inclusion guard is designed as some binutils-internal thing, and they didn't/don't expect other projects to directly use/include it.

I guess that was then and this is now, as bfd.h appears to be exported on at least some Linux distributions, and documented in a libbfd manual that is not marked in any obvious way as "binutils developer documentation" rather than "documentation for people using libbfd even in third-party projects".

And those who are will need to work around the guard by supply any config.h (or something that behaves like one) to stop it from complaining, even if that project isn't actually using autoconf.

Anything that defines the appropriate macros, even if it's an in-line #define, is "something that behaves like a config.h".

Now, if instrument-functions.c uses stuff from config.h or might use it in the future, then it should obviously be included there. If not, then just #defining stuff would also make sense.

However, having some project's public headers depend in any way whatsoever on stuff defined in a config.h file is a truly horrible mistake. My experience with one instance of that mistake, as detailed in a long comment in code where I had to deal with said mistake, has left me with an extremely strong distaste for public headers that depend on a config.h file.

(The problem there was eventually resolved by somebody replacing the code that depended on zlib's gz io routines with some sample code from zlib to support high-speed random access to gzipped files.)

[corubba]

Fedora also patches out that inclusion guard since 2012. While Debian does it in the install phase (after compilation), here it is done before building binutils. The comments for the patch in the Fedora package spec reflect the same sentiment from the binutils bugtracker.

Fun fact: Nick Clifton, who added that inclusion guard to bfd.h in 2012, also did and still does maintain the binutils package in Fedora.

[fxlb]

AlmaLinux

No problem on AlmaLinux 9, no guard in bfd.h.
Tested with docker run -it almalinux.

[fxlb]

Fedora

No problem on Fedora 42, no guard in bfd.h.
Tested with docker run -it fedora.

[fxlb]

Rocky Linux

No problem on Rocky Linux 8, no guard in bfd.h.
Tested with docker run -it rockylinux/rockylinux.

[fxlb]

CentOS

No problem on CentOS Linux 8, no guard in bfd.h.
Tested with docker run -it centos:centos8.

[fxlb]

Arch Linux maintainers should do the same removal of the guard...

[infrastation]

The distribution-specific patching and the project-specific workaround are often not mutually exclusive, as seems to be in this case: it would work to open a bug report in Arch Linux and to add a #define or an #include to libpcap with a reference to the related bug reports in binutils and Arch Linux.

$ git grep -c '/(view_bug|show_bug|ticket|issues)'
CHANGES:1
CMakeLists.txt:4
CONTRIBUTING.md:1
build.sh:1
configure.ac:4
pcap-bt-linux.c:2
pcap-common.c:1
pcap-haiku.c:2
pcap-linux.c:1
testprogs/findalldevstest.c:1
testprogs/valgrindtest.c:2

[corubba]

OpenSuse patches it out as well. OpenBSD doesn't ship (lib)bfd at all, NetBSD and FreeBSD keep it unpatched.

That inclusion guard is really accomplishing its goal when most distributions patch it out ...

[corubba]

I took the issue to the binutils mailing list, lets see what they think of it.