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

define pgm_read_glyph_ptr() and pgm_read_pointer() as static #591

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

Conversation

vortigont
Copy link

functions defined in header file (.h) must be defined as static, otherwise there could a be situations when linker fails with 'multiple definition' error'

i.e.

(.text+0x0): multiple definition of `_Z18pgm_read_glyph_ptrPK7GFXfonth'; .pio/build/hdwf2_my/src/actions.cpp.o (symbol from plugin):(.text+0x0): first defined here
(.text+0x0): multiple definition of `_Z19pgm_read_bitmap_ptrPK7GFXfont'; .pio/build/hdwf2_my/src/actions.cpp.o (symbol from plugin):(.text+0x0): first defined here

functions defined in header file (.h) must be defined as static, oherwise there could a situations when
linker fails with 'multiple definition' error'
@moononournation
Copy link
Owner

Can you reproduce it with a simple sample code compile in Arduino IDE?

@vortigont
Copy link
Author

there can't be a 100% reproducible case for this. It's very specific to used toolchain, compiler options, optimization and project structure. For me same project was building fine for a long time, but then after some toolchain update and/or refactoring I've started to see this issue. More to this same project could build fine via GitHub CI, so maybe OS is also might be a factor. I've narroved it down to your pgm_read_glyph_ptr() and pgm_read_pointer() functions are defined in header file so it breaks ODR unless compiler inlines it with internal linkage which is hinted by GFX_INLINE. Looks like there might be cases when compiler decides not to inline it within translation unit or inline externally and then linker fails. Defining it as static inline ensures internal linkage and won't disappoint linker.
It won't hurt anyway because if compiler inlines it internally - then fine, but if it decides to not inline it will link internally for TU. static is the key here and inline is just a hint for compiler which is still free to make the decision.
More hints could be found here and there

@moononournation
Copy link
Owner

If it should add static there should be many amendment not only these 2 place. Do you have any supporting document about adding static?

@vortigont
Copy link
Author

vortigont commented Dec 27, 2024

I do not think it would be required in any other places as I've checked the header files.
It only relates to functions that defined in headers. Functions declared in header and defined in .cpps are fine (i.e. those without {} body in .h). Also class member declarations are out of scope. So maybe this PR would be just fine unless I missed something in multiple display driver files.
You can check this post for a really good explanation about compiller's optimization options and inlining.

@moononournation
Copy link
Owner

this library have many function defined in header files, I need a deep review on it.

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

Successfully merging this pull request may close these issues.

2 participants