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

Add flash for vtable destination, make it default, and add build menu to control options #4582

Merged
merged 5 commits into from
Apr 2, 2018

Conversation

earlephilhower
Copy link
Collaborator

@earlephilhower earlephilhower commented Mar 27, 2018

Add an option for placing vtables in flash to complement the existing iram and heap options, and allow selection using a build menu. No makefile needed any more,.

Extension of #4551 issue and PR #4567

Add an option for placing vtables in flash to complement the existing
iram and heap options.  "make flash"

Now that there is a way to change it, move to vtables in flash as default
as only users with interrupts which use vtables require the vtable to
be in RAM.  For those users, if the tables are small enough they can put
them in IRAM and save heap space for their app.  If not, then the vtables
can be placed in HEAP which supports much larger tables.
@Adam5Wu
Copy link
Contributor

Adam5Wu commented Mar 28, 2018

@earlephilhower I am not nitpicking on you or this PR. :)

In my recent exploration solving "__c causes a section type conflict with __c" (#3369) I found it is possible to define multiple sections for string literals, so that data with different tags can coexist.

Then I just wonder, whether it is possible to do similar things with vtable data? That is:

  1. Put all vtable entries that are involved in ISR in one section, say .vtable.isr
  2. Put all other vtable entries in another section, say .vtable.regular
  3. Let the linker put .vtable.isr section in IRAM, and put .vtable.regular in flash

Just a crazy idea... not sure whether it is doable without hacking into the compiler.

@earlephilhower
Copy link
Collaborator Author

Unfortunately the compiler doesn't know which are interrupt called functions. I'm not sure you could tell that with static graph analysis, in fact. Turing strikes again!

@Adam5Wu
Copy link
Contributor

Adam5Wu commented Mar 28, 2018

But could human annotation help?

Just like I know I am writing a template class, so instead of PSTR("Blah"), I use PSTR_T("Blah") to put my string literal to a different section to avoid conflict.

I know I am writing a class for ISR, then I add some annotation to the class so compiler can place the vtable of this class to the .vtable.isr section.

If that is possible I am happy enough...

@earlephilhower
Copy link
Collaborator Author

I don't think there's a way to annotate other than by class names. You can individually specify in the .ld file to include, say, _ZL*__irq, in .RODATA (and hence in RAM), but I don't think I've ever seen any linker annotations on anything other than instances which do not have their own vtables.

If you find a way, we can figure out how to mangle it in, for sure! I think in practice, though, class functions in IRQs are pretty rare, and virtual tables in those are even less prevalent.

@devyte
Copy link
Collaborator

devyte commented Mar 31, 2018

Is there a way to be able to select this from the IDE menu? I'm thinking of the windows users.

@earlephilhower
Copy link
Collaborator Author

I think this can be in the menus if we change platform.txt and boards.py to build the app.ld file before every compile, like got GIT version.

In boards.txt there'll be (yet another) popup menu, identical for all devices, that adds a -D switch.

In platform.txt, add a recipe.hooks.prebuild.x to run the command the makefile does now.

I can give it a whirl, but it'll take a few days.

@d-a-v Does this make sense? You've been hacking the build.py and platform.txt much more than I have...

@d-a-v
Copy link
Collaborator

d-a-v commented Apr 1, 2018

Yes this could work this way and in fact would be much better because it'd work under windows without having to install make.

@devyte
Copy link
Collaborator

devyte commented Apr 1, 2018

@d-a-v that's exactly what I was getting at :)

@earlephilhower earlephilhower changed the title Add flash for vtable destination, make it default Add flash for vtable destination, make it default, and add build menu to control options Apr 2, 2018
@earlephilhower
Copy link
Collaborator Author

@d-a-v , @devyte I've just pushed a change which does what was discussed. A new menu is used instead of a makefile, and no tools are required other than GCC so it should run fine under Windows, too.

Copy link
Collaborator

@d-a-v d-a-v left a comment

Choose a reason for hiding this comment

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

works for me, thanks @earlephilhower and for the idea @devyte

@earlephilhower
Copy link
Collaborator Author

@d-a-v I'm stuck on how to make build.py get the default vtable flag that was added. Interactive builds seem to just work and take the first item by default, but the build.py script is barfing about it being undefined:

"/home/travis/arduino_ide/hardware/esp8266com/esp8266/tools/xtensa-lx106-elf/bin/xtensa-lx106-elf-gcc" -CC -E -P {build.vtable_flags} "/home/travis/arduino_ide/hardware/esp8266com/esp8266/tools/sdk/ld/eagle.app.v6.common.ld.h" -o "/home/travis/arduino_ide/hardware/esp8266com/esp8266/tools/sdk/ld/eagle.app.v6.common.ld"
xtensa-lx106-elf-gcc: error: {build.vtable_flags}: No such file or directory

@d-a-v
Copy link
Collaborator

d-a-v commented Apr 2, 2018

Setting the default value in platform.txt seems to solve it:

build.vtable_flags=-DVTABLES_IN_FLASH

It is overridden by the IDE.
(just verified both)

Convert from manual "make" operated app.ld creation to runtime creation
whose options are selected from the build menu.

Use a prelink recipe to create the output app.ld file each run, without
need for any special tools.

Update the boards.txt.py script to generate this new config.
@earlephilhower
Copy link
Collaborator Author

Thanks, @d-a-v . Looks to be building now!

@d-a-v d-a-v merged commit f2c7256 into esp8266:master Apr 2, 2018
@earlephilhower earlephilhower deleted the vtable_to_flash branch April 2, 2018 23:05
@@ -76,6 +78,9 @@ recipe.hooks.core.prebuild.2.pattern=bash -c "mkdir -p {build.path}/core && echo
recipe.hooks.core.prebuild.1.pattern.windows=cmd.exe /c mkdir {build.path}\core & (echo #define ARDUINO_ESP8266_GIT_VER 0x00000000 & echo #define ARDUINO_ESP8266_GIT_DESC win-{version} ) > {build.path}\core\core_version.h
recipe.hooks.core.prebuild.2.pattern.windows=

## Build the app.ld linker file
recipe.hooks.linking.prelink.1.pattern="{compiler.path}{compiler.c.cmd}" -CC -E -P {build.vtable_flags} "{runtime.platform.path}/tools/sdk/ld/eagle.app.v6.common.ld.h" -o "{runtime.platform.path}/tools/sdk/ld/eagle.app.v6.common.ld"
Copy link
Member

Choose a reason for hiding this comment

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

eagle.app.v6.common.ld is now auto-generated, why is it still tracked in Git? I think it should be removed from the repo and added to .gitignore.

INemesisI added a commit to RystaHub/Arduino that referenced this pull request Apr 10, 2018
… to control options (esp8266#4582)

* Add flash for vtable destination, make it default

Add an option for placing vtables in flash to complement the existing
iram and heap options.  "make flash"

Now that there is a way to change it, move to vtables in flash as default
as only users with interrupts which use vtables require the vtable to
be in RAM.  For those users, if the tables are small enough they can put
them in IRAM and save heap space for their app.  If not, then the vtables
can be placed in HEAP which supports much larger tables.

* Add VTable menu, FLASH as default, remove Makefile

Convert from manual "make" operated app.ld creation to runtime creation
whose options are selected from the build menu.

Use a prelink recipe to create the output app.ld file each run, without
need for any special tools.

Update the boards.txt.py script to generate this new config.
@innodron
Copy link

I have just pulled in the most recent commit (e3c97021) yesterday and now I am getting xtensa-lx106-elf/bin/ld: cannot open linker script file ../ld/eagle.app.v6.common.ld: No such file or directoryerror when compiling. Apparently this file is now removed from the repo and expected to be produced during a sketch build.

However, It seems eagle.app.v6.common.ld is not being auto-generated for some reason in my case. I will appreciate to get guidance on how to approach to this issue. Apperently, no similar problems reported yet.

I am using Eclipse with Sloeber plugin. This is an ESP-12F module with the attached properties. Frankly I am not sure which value the new VTables option should take; I just assigned the default.

@d-a-v
Copy link
Collaborator

d-a-v commented May 18, 2018

The command line is in platform.txt.
You can do it by hand using the default option with:

tools/xtensa-lx106-elf/bin/xtensa-lx106-elf-gcc -DVTABLES_IN_FLASH -CC -E -P tools/sdk/ld/eagle.app.v6.common.ld.h -o tools/sdk/ld/eagle.app.v6.common.ld

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.

6 participants