Skip to content

Fixes issues found when compiling for fvm with gcc#154

Open
wasv wants to merge 7 commits intoflipper-io:masterfrom
wasv:wasv_fvm_fix
Open

Fixes issues found when compiling for fvm with gcc#154
wasv wants to merge 7 commits intoflipper-io:masterfrom
wasv:wasv_fvm_fix

Conversation

@wasv
Copy link
Collaborator

@wasv wasv commented Jul 15, 2018

Adds header to posix includes for missing pio macros.
Fixes parameter ordering issues in flipper.mk.
Fixes section naming issue due to clang vs gcc differences.
Updated flipper.mk to fix missing c bindings in fvm binary.

wasv added 3 commits July 15, 2018 00:52
Adds header for missing macros.
Fixes parameter ordering issues in flipper.mk.
Fixes section naming issue due to clang vs gcc differences.
@wasv wasv requested a review from georgemorgan July 17, 2018 23:35
@@ -0,0 +1,296 @@
/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file shouldn’t be added here. There must be something wrong with the install or the file that needs this include. Did you define ATSAM4S at the top?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

When building the app for fvm, the macros for PIO_P* are referenced. This is here to provide those macros for the POSIX platform headers. Should I define ATSAM4S in posix.h?

Copy link
Collaborator

Choose a reason for hiding this comment

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

A build for posix should still have access to the SAM headers, as they are installed. I can build the app for FVM fine on macOS so I think we have another case of this doesn't d.t.r.t on Linux.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Check build/include to see what's copied to $PREFIX/include. IIRC PIO.h is under flipper/atsam4s/asf somewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How can both the posix and atsam4s headers be included? The carbon.h file included in flipper.h will include the atsam4s headers if ATSAM4S is defined, but then it won't include the posix headers. Based on lines 7-15 of flipper/carbon.h.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I just pushed 11fc8c6, which gets the app to compile, but still not link, for me. Does that work on your end?


/* Declare the LF_VAR and LF_FUNC types for this platform. */
#define LF_VAR __attribute__((section("__DATA,.lf.vars")))
#define LF_FUNC __attribute__((section("__TEXT,.lf.funcs")))
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will break macOS. Need this here for clang because of how section names work.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

a6f261d should fix this.

@georgemorgan
Copy link
Collaborator

Hey @wastevensv, can you confirm that these issues are still present in the dyld branch? I think most everything has been fixed. If not, we can rebase this PR and merge.

@wasv
Copy link
Collaborator Author

wasv commented Oct 27, 2018

Sure, I can rebuild the dyld branch and see what happens. I'll try and get to that this weekend.

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

Comments