Skip to content

Conversation

@willcl-ark
Copy link
Member

@willcl-ark willcl-ark commented Oct 10, 2023

This is one way to fix #28600

Recently usage of undeclared functions became an error rather than a warning, in C2x. https://reviews.llvm.org/D122983?id=420290

This change has migrated into the build tools of Ubuntu 23.10 which now causes the USDT tests to fail to compile, see
#28600

I think there are various potential fixes:

  1. Manually declare the functions we use
  2. Fix imports so that manual declarations aren't needed
  3. Revert the new C2X behaviour and don't error on implicit function declarations

I would have preferred solution 2, but I believe this will require changes to the upstream bcc package. Having played with the imports I can get things working in a standalone C program, using system headers, but when building the program from a python context as we do in the test it uses its own headers (bundled with the python lib) rather than the system ones, and manually importing (some) system headers results in definition mismatches. I also investigated explicitly importing required headers from the package, which use paths like #import </virtual/bcc/bcc_helpers.h>, but this seems more obtuse and brittle than simply ignoring the warning.

Therefore I think that until the upstream python pacakge fixes their declarations, we should fix this by setting -Wno-error=implicit-function-declaration for the tracing programs.

cc maflcko 0xB10C

Recently usage of undeclared functions became an error rather than a
warning, in C2x. https://reviews.llvm.org/D122983?id=420290

This change has migrated into the build tools of Ubuntu 23.10 which now
causes the USDT tests to fail to compile, see
bitcoin#28600

Fix this by setting `-Wno-error=implicit-function-declaration` for the
tracing programs.
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 10, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK maflcko

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Tests label Oct 10, 2023
@fanquake
Copy link
Member

Therefore I think that until the upstream python pacakge fixes their declarations,

Is there an upstream issue tracking this?

@willcl-ark
Copy link
Member Author

Therefore I think that until the upstream python pacakge fixes their declarations,

Is there an upstream issue tracking this?

Not to my knowledge. The closest is iovisor/bcc#4740

I'll open one over there (and perhaps get clarification that this is something that they can/will fix on their side).

@willcl-ark
Copy link
Member Author

OK I opened iovisor/bcc#4757

@maflcko
Copy link
Member

maflcko commented Oct 11, 2023

lgtm ACK 4077e43

CI passed on this, and it should be trivial to revert this temporary workaround.

I may test after merge.

@maflcko maflcko added this to the 26.0 milestone Oct 11, 2023
@fanquake
Copy link
Member

Seems fine for now as a workaround. Can follow up if there is activity upstream.

@fanquake fanquake merged commit 06d469c into bitcoin:master Oct 12, 2023
@willcl-ark willcl-ark deleted the mantis-usdt-fix branch October 12, 2023 10:04
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Oct 13, 2023
4077e43 test: fix usdt undeclared function errors on mantis (willcl-ark)

Pull request description:

  This is one way to fix bitcoin#28600

  Recently usage of undeclared functions became an error rather than a warning, in C2x. https://reviews.llvm.org/D122983?id=420290

  This change has migrated into the build tools of Ubuntu 23.10 which now causes the USDT tests to fail to compile, see
  bitcoin#28600

  I think there are various potential fixes:

  1. Manually declare the functions we use
  2. Fix imports so that manual declarations aren't needed
  3. Revert the new C2X behaviour and don't error on implicit function declarations

  I would have preferred solution 2, but I believe this will require changes to the upstream bcc package. Having played with the imports I can get things working in a standalone C program, using system headers, but when building the program from a python context as we do in the test it uses its own headers (bundled with the python lib) rather than the system ones, and manually importing (some) system headers results in definition mismatches. I also investigated explicitly importing required headers from the package, which use paths like `#import </virtual/bcc/bcc_helpers.h>`, but this seems more obtuse and brittle than simply ignoring the warning.

  Therefore I think that until the upstream python pacakge fixes their declarations, we should fix this by setting `-Wno-error=implicit-function-declaration` for the tracing programs.

  cc maflcko 0xB10C

ACKs for top commit:
  maflcko:
    lgtm ACK 4077e43

Tree-SHA512: 8368bb1155e920a95db128dc893267f8dab64f1ae53f6d63c6d9294e2e4e92bef8515e3697e9113228bedc51c0afdbc5bbcf558c119bf0eb3293dc2ced86b435
@0xB10C
Copy link
Contributor

0xB10C commented Oct 16, 2023

Post-merge lgtm ACK. Thanks for tackling this. I'll update #25832 with the same flags.

@bitcoin bitcoin locked and limited conversation to collaborators Oct 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ci: USDT on mantic

5 participants