Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix include problems in thirdparty/jpeg-compressor/jpge.cpp #101927

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

StefanCristian
Copy link

@StefanCristian StefanCristian commented Jan 22, 2025

…ue to clang being built with FORTIFY_SOURCE>=1 on some systems. Stdio.h should have already been up there anyway, if used.

I'm compiling GoDot source via FORTIFY_SOURCE=2 clang (via Gentoo profiles), and went through the same exact issue as the one reported in the OpenEmbedded channel:

In file included from thirdparty/jpeg-compressor/jpge.cpp:936:
In file included from /usr/lib/clang/19/include/llvm_libc_wrappers/stdio.h:13:
In file included from /usr/include/stdio.h:970:
/usr/include/bits/stdio2.h:128:13: error: use of undeclared identifier '__builtin___vfprintf_chk'; did you mean '__builtin___sprintf_chk'?
  128 |   int __r = __builtin___vfprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1,
      |             ^
/usr/include/bits/stdio2.h:128:39: error: cannot initialize a parameter of type 'char *' with an lvalue of type 'FILE *const __restrict' (aka 'jpge::_IO_FILE *const __restrict')
  128 |   int __r = __builtin___vfprintf_chk (__stream, __USE_FORTIFY_LEVEL - 1,

This is done via Clang 19.1.4, with makefile scons options:

scons platform=linuxbsd target=editor production=true use_llvm=yes linker=lld compiledb=yes precision=single debug_symbols=true pulseaudio=true dbus=true fontconfig=true -j 22

…ue to clang being built with FORTIFY_SOURCE>=1 on some systems. Stdio.h should have already been up there anyway, if ever used.
@StefanCristian StefanCristian requested a review from a team as a code owner January 22, 2025 18:38
@AThousandShips
Copy link
Member

AThousandShips commented Jan 22, 2025

Thank you for your contribution, however thirdparty code shouldn't generally be modified directly

In cases where it is absolutely necessary a patch should be added

This needs an issue to track the problem though

A unknown developer from Renderdoc rejected the fix without considering the solution, and based on the fact that the Clang upstream should fix it first

The fact that the thirdparty developer rejected this fix is not a good sign IMO, this needs more context

@AThousandShips AThousandShips changed the title thirdparty/jpeg-compressor/jpge.cpp: workaround reordering stdio.h, d… Fix include problems in thirdparty/jpeg-compressor/jpge.cpp Jan 22, 2025
@AThousandShips AThousandShips added this to the 4.x milestone Jan 22, 2025
@StefanCristian
Copy link
Author

Thank you for your contribution, however thirdparty code shouldn't generally be modified directly

In cases where it is absolutely necessary a patch should be added

This needs an issue to track the problem though

A unknown developer from Renderdoc rejected the fix without considering the solution, and based on the fact that the Clang upstream should fix it first

The fact that the thirdparty developer rejected this fix is not a good sign IMO, this needs more context

I completely agree, yet the context of the rejection is questionable, even though normally upstream Clang should have this fixed, not the thirdparty developer.

This is as you decide if it's worth to add, but it's completely understandable 3rd parties are just cloned.

I have set the context from OpenEmbedded report.

@AThousandShips
Copy link
Member

Not my decision to make but just some pointers! But please do add some more context to that rejection, it would be really relevant

This should also be suggested upstream if this repo is still active (I have a vague memory that this particular one isn't), but it should at least be tracked in Clang so would be good to see what progress is being made if any on the part of llvm to solve this

@StefanCristian
Copy link
Author

Not my decision to make but just some pointers! But please do add some more context to that rejection, it would be really relevant

This should also be suggested upstream if this repo is still active (I have a vague memory that this particular one isn't), but it should at least be tracked in Clang so would be good to see what progress is being made if any on the part of llvm to solve this

I have updated the original message to provide the full context.
Cheers!

@fire
Copy link
Member

fire commented Jan 22, 2025

This doesn't look that difficult, but we need to reference the documents for making patches. I've forgotten the steps already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants