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 liblvgl with monolith #735

Open
wants to merge 4 commits into
base: develop-pros-4
Choose a base branch
from
Open

fix liblvgl with monolith #735

wants to merge 4 commits into from

Conversation

Rocky14683
Copy link
Member

@Rocky14683 Rocky14683 commented Dec 27, 2024

Summary:

fix liblvgl with monolith

also delete make template overload(That's just a small bug)

Motivation:

#728

Test Plan:

  • Hot/cold
  • monolith
  • More tests, just in case I'm stupid

also delete make template overload(Thats just a small bug)
Copy link
Contributor

@djava djava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused about what is happening here, can you explain what these changes actually are?

And should this really go here instead of in liblvgl?

@@ -75,7 +75,7 @@ CREATE_TEMPLATE_ARGS+=--user "src/main.{cpp,c,cc}" --user "include/main.{hpp,h,h
CREATE_TEMPLATE_ARGS+=--target v5
CREATE_TEMPLATE_ARGS+=--output bin/monolith.bin --cold_output bin/cold.package.bin --hot_output bin/hot.package.bin --cold_addr 58720256 --hot_addr 125829120

template:: patch_sdk_headers clean-template library
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure this was a mistake? It has a semantically different meaning now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeh, its just a kernel thing. If it overload it will duplicate a libpros.a in the project root directory.

Copy link
Contributor

@djava djava Dec 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the template rule in common.mk is no longer used then, since it's being overridden in user project Makefile? Can it be removed, or is it used to build the kernel? If so, should it be in the kernel's Makefile?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's only being used by users. There's no template rule in the distributed makefile, only in common.mk.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so the kernel build uses this template rule here, and user project builds use the one in common.mk? Then shouldn't the one in common.mk be moved to template-Makefile? If it's not used by both, it shouldnt be in common.mk, right?

common.mk Outdated
Comment on lines 46 to 47
LNK_FLAGS=--gc-sections --start-group $(strip $(LIBRARIES)) -lgcc -lstdc++ --end-group -T$(FWDIR)/v5-common.ld --no-warn-rwx-segments
LNK_FLAGS_MONOLITH=--gc-sections --start-group $(strip $(LVGL_LIB_FLAGS) $(FILTERED_LIBRARIES)) -lgcc -lstdc++ --end-group -T$(FWDIR)/v5-common.ld --no-warn-rwx-segments
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we figure out a better way to do this than duplication? Also should LNK_FLAGS be LNK_FLAGS_HOTCOLD or something?

@Rocky14683
Copy link
Member Author

Yes it should be in kernel because it's just linking issue.

@Rocky14683
Copy link
Member Author

I make it --whole-archive so that every symbol in liblvgl are included and are strong ref.

@djava
Copy link
Contributor

djava commented Dec 27, 2024

Could I have an explanation of what the linking issue is and how this solves it? And also probably set it as the top-level PR description,

@Rocky14683
Copy link
Member Author

The linking process didn't work initially because the linker was discarding unused symbols from liblvgl.a. Adding --whole-archive works because it forces the linker to include all symbols from liblvgl.a, even if the linker doesn't detect any direct references to those symbols.

@Rocky14683
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@djava
Copy link
Contributor

djava commented Dec 28, 2024

Ok, can you add that explanation as a comment in the makefile?

@SizzinSeal
Copy link
Contributor

So, if I'm reading this correctly, the entirety of liblvgl is put in the monolith? The binary size is going to balloon.

Additionally, what if one generic template overrides a weakly declared symbol in another template? Is that going to work? The changes in this PR is specific to liblvgl

@Rocky14683
Copy link
Member Author

  1. I didn't think about the binary size
  2. It should still work

@SizzinSeal
Copy link
Contributor

PR #728 also fixes the problem, without modifications to common.mk which is specific to LVGL, and without causing the binary size to explode. What's the motivation for this PR?

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.

3 participants