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 linker error when using Os optimization #669

Open
wants to merge 2 commits into
base: trunk
Choose a base branch
from

Conversation

maxgerhardt
Copy link

@maxgerhardt maxgerhardt commented Jan 19, 2025

I noticed this bug when compiling libdragon with my own build scripts, which used -Os instead of -O2.

This generated a final linking error of

C:/Users/Max/.platformio/packages/toolchain-gccmips64/bin/../lib/gcc/mips64-elf/14.2.0/../../../../mips64-elf/bin/ld.exe: .pio\build\n64\FrameworkLibdragon\display.o: in function `display_init':
(.text.display_init+0x360): undefined reference to `vi_write_config'

I then noticed that vi_write_safe() and vi_write_config() were the only two functions that were inline instead of static inline.

After adding the static, the linker error went away.

Edit: Corrected a similiar error in include/n64sys.h.

@rasky
Copy link
Collaborator

rasky commented Jan 25, 2025

Hi, thanks for this. Adding static unfortunately means that the function disappears from doxygen. The workaround used in other places is to declare the functions as "extern inline" in the C file. Grep for it to see how it's done.

@maxgerhardt
Copy link
Author

So the other static inline functions that do have doxygen comments on them like

libdragon/src/vi.h

Lines 241 to 251 in e037067

/**
* @brief Update the framebuffer pointer in the VI
*
* @param[in] dram_val
* The new framebuffer to use for display. Should be aligned and uncached.
*/
static inline void vi_write_dram_register( void const * const dram_val )
{
*VI_ORIGIN = VI_ORIGIN_SET(PhysicalAddr(dram_val));
MEMORY_BARRIER();
}

are okay to keep as static? I'm not quite sure how the distinciton should be made between static inline being okay or not. Other files like surface.h also have tons of only inline functions, I wonder if that breaks too with -Os or -Og/-O0 optimization, need to check.

@rasky
Copy link
Collaborator

rasky commented Jan 26, 2025

So the other static inline functions that do have doxygen comments on them like

Even if they do, they still don't show up in doxygen:
https://libdragon.dev/ref/vi_8h.html

are okay to keep as static? I'm not quite sure how the distinciton should be made between static inline being okay or not.

There is no distinction basically. You should never use static inline in a public header of libdragon, because that would not be documented. You can still use static inline in lidbragon .c files because those are far less important. We still generate doxygen for them for reasons, but we don't care if some functions are missing.

Other files like surface.h also have tons of only inline functions, I wonder if that breaks too with -Os or -Og/-O0 optimization, need to check.

They won't break because they are defined as extern inline as I advised you to do:

libdragon/src/surface.c

Lines 75 to 78 in e037067

extern inline surface_t surface_make(void *buffer, tex_format_t format, uint16_t width, uint16_t height, uint16_t stride);
extern inline tex_format_t surface_get_format(const surface_t *surface);
extern inline surface_t surface_make_linear(void *buffer, tex_format_t format, uint16_t width, uint16_t height);
extern inline bool surface_has_owned_buffer(const surface_t *surface);

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