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

Enable libstdc++ v2 #146

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

Conversation

Sermus
Copy link
Contributor

@Sermus Sermus commented Apr 1, 2016

This is a reworked pull requiest for #144 after comments made by @pfalcon and @jcmvbkbc.
The commits are better structured.
The STL SGI tests are not included in this request because there is no way to build 1MB+ images in current version of esp-open-sdk.

@susisstrolch
Copy link

Seems the patches aren't sufficient to get esp_stdcpp_port/ and esp_newlib_port/ populated by git submodule {sync|update}...
So, can you please explain how to add them to the submodule tree?

$ git submodule 
 46f160f7cdb6b993afca1a685ddff5b3aec656b6 crosstool-NG (crosstool-ng-1.22.0-53-g46f160f)
 5bd20afa6869b682001250e2a95c261390f1098d esp-open-lwip (remotes/origin/sdk-1.5.0)
-ab0b40c677d3f914533a1e1f98f422bc1bade68d esp_newlib_port
-d3008f47d087b1ad6e42ca135cf27f27c5845e16 esp_stdcpp_port
 5eb1563501211b6661d00582eb280df0c96e3f82 esptool (remotes/origin/esp-open-sdk)
 e4bcc63c9c016e4f8848e7e8f512438ca857531d lx106-hal (heads/master)

@Sermus
Copy link
Contributor Author

Sermus commented Apr 2, 2016

Not sure what exactly you're trying to do. Cloning with recursive submodule checkout seems to work fine:
git clone --recursive -b enable_libstdc++_v2 https://github.com/Sermus/esp-open-sdk.git

@susisstrolch
Copy link

For YOUR repo - yes.
But not when applying this pull request against pfalcon/esp-open-sdk:
Having a clean local clone from pfalcon/esp-open-sdk.
Applying your pull request localy:

git fetch origin pull/146/head:libstdc++v2
git checkout libstdc++v2
git submodule sync
git submodule update

Result: as shown in my last message - your submodules aren't pulled in, so the build will fail badly:

...
Installing vendor SDK headers into toolchain sysroot
Installing vendor SDK libs into toolchain sysroot
Installing vendor SDK linker scripts into toolchain sysroot
Installing port lib headers into toolchain sysroot
cp: cannot stat ‘esp_stdcpp_port/cpp_routines.h’: No such file or directory
make: *** [standalone] Error 1

@Sermus
Copy link
Contributor Author

Sermus commented Apr 2, 2016

Shouldn't have you used "git submodule update --init" since these two are new to your .git/config?

@susisstrolch
Copy link

Thanks!

@Sermus Sermus mentioned this pull request Apr 5, 2016
@jeremyd2019
Copy link
Contributor

I've started trying to play with this. So far I have run into a few issues

  • I would prefer do_global_ctors() to be declared extern "C" (or just defined in a .c file), because I want to call it from my user_main.c file. It may just be me, but it feels wrong to be 'in' c++ before the global constructors have been called, since the global constructors in a translation unit are supposed to have been called by the time a call into that translation unit runs:

It is implementation-defined whether or not the dynamic initialization (8.5, 9.4, 12.1, 12.6.1) of an object of namespace scope is done before the first statement of main. If the initialization is deferred to some point in time after the first statement of main, it shall occur before the first use of any function or object defined in the same translation unit as the object to be initialized.

  • Attempting to link -lstdc++port is causing me duplicate symbol errors:
xtensa-lx106-elf/sysroot/usr/lib\libstdc++port.a(cpp_routines.o): In function `operator new(unsigned int)':
cpp_routines.cpp:(.text._Znwj+0x0): multiple definition of `operator new(unsigned int)'
xtensa-lx106-elf/sysroot/lib\libstdc++irom.a(new_op.o):esp-open-sdk/crosstool-NG/.build/src/gcc-4.8.5/libstdc++-v3/libsupc++/new_op.cc:45: first defined here
xtensa-lx106-elf/bin/../xtensa-lx106-elf/sysroot/usr/lib\libstdc++port.a(cpp_routines.o): In function `operator delete(void*)':
cpp_routines.cpp:(.text._ZdlPv+0x0): multiple definition of `operator delete(void*)'
xtensa-lx106-elf/bin/../xtensa-lx106-elf/sysroot/lib\libstdc++irom.a(del_op.o):esp-open-sdk/crosstool-NG/.build/src/gcc-4.8.5/libstdc++-v3/libsupc++/del_op.cc:45: first defined here
  • It appears that libstdc++irom.a did not actually wind up having its symbols moved to the .irom.text section. I believe this is due to libstdc++ having been built with -ffunction-sections -fdata-sections, and the objcopy rename section only matching exact section names (not the prefix, i.e. only .text not .text.somefunction sections get renamed).

@Sermus
Copy link
Contributor Author

Sermus commented Apr 6, 2016

Thanks for the feedback. I started looking at the issue 1 and end up by reworking stdcpp_port throwing away garbage no more needed there. This fixed first two issues from your comment. Please pull in updates.

As for the third one, you're right. libstdc++ is built sectionized and objcopy doesn't move content of those sections to .irom.text. I didn't find a way to make objcopy do such thing because it doesn't accept wildcards in section name specifier. Doing this section-by-section looks overexpensive. This is the reason why i created romized versions of linker scripts which make .text.* and .literal.* sections go to irom during link phase. If you have a better option please let me know.

@jeremyd2019
Copy link
Contributor

I played with this a little and had some success by modifying the linker script as follows to just put text and literal sections from libstdc++irom.a into irom. I noticed you were already doing something similar with libm.a in your ld-script-irom.patch.

--- xtensa-lx106-elf/xtensa-lx106-elf/sysroot/usr/lib/eagle.app.v6.ld   2016-04-06 10:29:05.166730000 -0700
+++ xtensa-lx106-elf/xtensa-lx106-elf/sysroot/usr/lib/eagle.app.v6.test.ld  2016-04-06 22:57:08.686971600 -0700
@@ -102,11 +102,14 @@
     *(.gnu.linkonce.e.*)
     *(.gnu.version_r)
     *(.eh_frame)
+    . = (. + 3) & ~ 3;
     /*  C++ constructor and destructor tables, properly ordered:  */
+    __init_array_start = ABSOLUTE(.);
     KEEP (*crtbegin.o(.ctors))
     KEEP (*(EXCLUDE_FILE (*crtend.o) .ctors))
     KEEP (*(SORT(.ctors.*)))
     KEEP (*(.ctors))
+    __init_array_end = ABSOLUTE(.);
     KEEP (*crtbegin.o(.dtors))
     KEEP (*(EXCLUDE_FILE (*crtend.o) .dtors))
     KEEP (*(SORT(.dtors.*)))
@@ -151,6 +154,14 @@
   } >dram0_0_seg :dram0_0_bss_phdr
 /* __stack = 0x3ffc8000; */

+  .irom0.text : ALIGN(4)
+  {
+    _irom0_text_start = ABSOLUTE(.);
+    *(.irom0.literal .irom.literal .irom.text.literal .irom0.text .irom.text)
+    *libstdc++irom.a:(.literal .text .literal.* .text.*)
+    _irom0_text_end = ABSOLUTE(.);
+  } >irom0_0_seg :irom0_0_phdr
+
   .text : ALIGN(4)
   {
     _stext = .;
@@ -198,13 +209,6 @@
     *(.gnu.linkonce.lit4.*)
     _lit4_end = ABSOLUTE(.);
   } >iram1_0_seg :iram1_0_phdr
-
-  .irom0.text : ALIGN(4)
-  {
-    _irom0_text_start = ABSOLUTE(.);
-    *(.irom0.literal .irom.literal .irom.text.literal .irom0.text .irom.text)
-    _irom0_text_end = ABSOLUTE(.);
-  } >irom0_0_seg :irom0_0_phdr
 }

 /* get ROM code address */

@jeremyd2019
Copy link
Contributor

Perhaps it would even make sense to put *irom.a:(.text .literal .text.* .literal.*) in there, and then the user could just (re)name libs they wanted to put in the irom section libsomethingirom.a.

@Sermus
Copy link
Contributor Author

Sermus commented Apr 7, 2016

Yeah, this would work. At the same time i'm really in doubt even introducing the original changes into the linker scripts. Generally, doing so we bury a requirement to how the user application shall be built into a linker script, this smells bad. On the other hand, we seem to have no way out. Either we do this way or we don't do at all.

@Sermus
Copy link
Contributor Author

Sermus commented Apr 7, 2016

Now i have a particular case of using this buried knowledge. I tried to build SGI STL sample and fail to link because implementations generated from templates are quite big and wouldn't fit into iram. The fix was quite easy: i preliminarily packed the compiler objects into sgitestsirom.a so that it applies to the linker script rule. But i can admit than this is definitely better than making .text.* go to irom I initially introduced in the linker scripts. So, thanks for the feedback, i'll incorporate your suggestion into the linker script patch.

@jeremyd2019
Copy link
Contributor

I agree on not being overly fond of making a magic name trigger this behavior.
Learning from investigating this, I think I am of the opinion that any reasonably complex project would probably need to make its own custom ld script to make sure everything goes where it needs to. The magic name trick might be enough for a simple project, or to get somebody started when they run out of iram.

@Sermus
Copy link
Contributor Author

Sermus commented Apr 8, 2016

Yes, exactly. This gives at least a good hint where to go when you face such an issue.

@Sermus
Copy link
Contributor Author

Sermus commented Apr 8, 2016

Just for information. I did a build test for Micropython project with the new environment. The built binary is different from what is built with the current esp-open-sdk head revision (mainly because gcc changed from 4.8.2 to 4.8.5 I suppose). The binary is loaded into esp8266. It seems to work as expected. At least print("Hello, World"); does what it has to.

@pfalcon
Copy link
Owner

pfalcon commented Apr 8, 2016

@Sermus: Great, thanks! I don't have time to look in more detail at your patches currently, but my comments would be exactly along the way of "how many projects with good testsuite coverage were tested with these patches?"

@Sermus
Copy link
Contributor Author

Sermus commented Apr 8, 2016

Can you recommend more or less representative project set you usually use for such purposes? I'll do my best to check if they are ok with my changes.


$(TOOLCHAIN)/xtensa-lx106-elf/sysroot/lib/libstdc++irom.a: $(TOOLCHAIN)/xtensa-lx106-elf/sysroot/lib/libstdc++.a $(TOOLCHAIN)/bin/xtensa-lx106-elf-gcc
@echo "Creating irom version of libstdc++..."
$(TOOLCHAIN)/bin/xtensa-lx106-elf-objcopy --rename-section .text=.irom0.text \
Copy link
Contributor

Choose a reason for hiding this comment

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

Since it has pretty much been established that this objcopy doesn't do anything useful, might it not be more efficient to just use cp instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tend to agree, but there is one argument against this approach. I tried to keep backward compatibility meaning the patch mustn't break solutions which worked before. If we change libstdc++irom just to a copy we either keep inconsistency between the way how libcirom is built and the way how libstdc++irom is built or we may break those solutions which used libcirom assuming that the functions inside are packed into .irom0.text.
So i'd rather keep it the way it's done now to keep backward compatibility and unified way of building *irom libs.

@Sermus
Copy link
Contributor Author

Sermus commented May 1, 2016

Hello Paul,
I ran micropython's test suites for the versions built with the mainstream toolchain and with the c++-enabled one. The results are identical.

For me the test suite reports:
412 tests performed (13321 individual testcases)
412 tests passed
48 tests skipped: builtin_compile class_descriptor exception_chain fun_name ordereddict1 parser slice_attrs string_splitlines machine1 machine_mem urandom_extra vfs_fat_ramdisk bytearray_construct bytes_construct cmath_fun cmath_fun_special complex1 float2int float2int_doubleprec float_divmod int_big_float math_fun_special string_format true_value types meminfo memstats native_closure native_const native_misc viper_addr viper_args viper_binop_arith viper_binop_comp viper_binop_comp_imm viper_binop_multi_comp viper_cond viper_error viper_misc viper_ptr16_load viper_ptr16_store viper_ptr32_load viper_ptr32_store viper_ptr8_load viper_ptr8_store viper_subscr rge_sm sys_exc_info

@pfalcon
Copy link
Owner

pfalcon commented May 1, 2016

Thanks for testing that, your contribution and your patience! We should get initial release of MicroPython esp8266 next week, and that should unfreeze changes to esp-open-sdk. It may still take some time (and there're other things in queue first like upgrade to SDK 1.5.3), but we're getting there.

balazsracz and others added 2 commits May 9, 2016 21:39
Need additional tool to be installed before running the make.
@Sermus
Copy link
Contributor Author

Sermus commented May 13, 2016

Hi Paul,
Is there any plan for integration window? Should I do upmerge?

@pfalcon
Copy link
Owner

pfalcon commented May 13, 2016

What do you mean by upmerge? Definitely please rebase your branch against master when you have a chance (github shows there're conflicts). Unfortunately, I'm still backlogged on looking into this. Well, your patch being merged into any open-source project means that a maintainer is ready to maintain your code, or that you yourself will be around to answer any issues. Both take time, and thanks for doing the 2nd part ;-). I hope to have some progress on the weekend (at least create a separate branch and merge something there).

@Sermus
Copy link
Contributor Author

Sermus commented May 13, 2016

Yes, i meant to say pulling recent changes from master in and resolving conflicts. The reason for the question was my natural laziness. I wouldn't like to do the same twice meaning if i rebase now and the request won't be merged anytime soon after most likely i'll be forced to rebase once again. That's why i wonder when you're ready to pull in the changes. Then i'll rebase and do final clearence.

@pfalcon
Copy link
Owner

pfalcon commented May 13, 2016

Feel free to put it off for now.

@Sermus
Copy link
Contributor Author

Sermus commented May 13, 2016

Sorry for making noise around this, but how do i know when this should be done? The only way i see is polling you from time to time. Last time you gave clear evidence when the next poll shall be performed.

@pfalcon
Copy link
Owner

pfalcon commented May 13, 2016

I'll write something here.

@pfalcon
Copy link
Owner

pfalcon commented May 13, 2016

Ok, let's start like this: https://github.com/pfalcon/esp-open-sdk/commits/gcc-4.8.5 . Feel free to rebase against that branch at your convenience. No hurry - I'll need to test switch to gcc 4.8.5 well first, then proceed with the rest. Thanks.

@Sermus
Copy link
Contributor Author

Sermus commented May 20, 2016

I'll wait until you backmerge this branch to master.
Meantime let me say that this pull request is (thus my results for Micropython are) already based on 4.8.5 in configuration as follows:

Using built-in specs.
COLLECT_GCC=xtensa-lx106-elf-gcc
COLLECT_LTO_WRAPPER=/opt/lx106/bin/../libexec/gcc/xtensa-lx106-elf/4.8.5/lto-wrapper
Target: xtensa-lx106-elf
Configured with: /home/andrey/esp-open-sdk/crosstool-NG/.build/src/gcc-4.8.5/configure --build=x86_64-build_pc-linux-gnu --host=x86_64-build_pc-linux-gnu --target=xtensa-lx106-elf --prefix=/home/andrey/esp-open-sdk/xtensa-lx106-elf --with-local-prefix=/home/andrey/esp-open-sdk/xtensa-lx106-elf/xtensa-lx106-elf/sysroot --with-sysroot=/home/andrey/esp-open-sdk/xtensa-lx106-elf/xtensa-lx106-elf/sysroot --with-newlib --enable-threads=no --disable-shared --with-pkgversion='crosstool-NG crosstool-ng-1.22.0-52-g58ee306' --disable-__cxa_atexit --enable-cxx-flags='-fno-exceptions -fno-rtti' --with-gmp=/home/andrey/esp-open-sdk/crosstool-NG/.build/xtensa-lx106-elf/buildtools --with-mpfr=/home/andrey/esp-open-sdk/crosstool-NG/.build/xtensa-lx106-elf/buildtools --with-mpc=/home/andrey/esp-open-sdk/crosstool-NG/.build/xtensa-lx106-elf/buildtools --with-isl=/home/andrey/esp-open-sdk/crosstool-NG/.build/xtensa-lx106-elf/buildtools --with-cloog=/home/andrey/esp-open-sdk/crosstool-NG/.build/xtensa-lx106-elf/buildtools --with-libelf=/home/andrey/esp-open-sdk/crosstool-NG/.build/xtensa-lx106-elf/buildtools --enable-lto --enable-target-optspace --without-long-double-128 --disable-libgomp --disable-libmudflap --disable-libssp --disable-libquadmath --disable-libquadmath-support --disable-nls --disable-multilib --enable-languages=c,c++
Thread model: single
gcc version 4.8.5 (crosstool-NG crosstool-ng-1.22.0-52-g58ee306)

@pfalcon
Copy link
Owner

pfalcon commented May 20, 2016

Meantime let me say that this pull request is (thus my results for Micropython are) already based on 4.8.5

Sure, and https://github.com/pfalcon/esp-open-sdk/commits/gcc-4.8.5 merges 1st patch from your series (that's why I mentioned need to tweak commit message to follow standard git commit message conventions (1st line fits in below than 80 chars, etc.))

thus my results for Micropython are

As I mentioned above, there can't be too much testing, only too little. For example, you probably didn't test that functions in bootrom 100% match interface and semantics of those in gcc 4.8.5's libgcc. And MicroPython started in gradual fashion to do what nobody else seems to do - leverage these high-performance functions while saving iRAM, and I don't want to block further progress. Well, maybe @jcmvbkbc can help here too: what probability of breaking changes in libgcc between gcc micro-versions? (I understand it's low, but I'd like a second opinion anyway). Then for future cases, what's this probability for minor gcc version upgrades? And for major? Maybe this interface is set in stone (due to its utter simplicity) and breaking changes is a truly exceptional once-in-a-life case?

@jcmvbkbc
Copy link
Contributor

what probability of breaking changes in libgcc

I only see operation helpers exported from the ROM, AFAIU their interface is set in stone.

@pfalcon
Copy link
Owner

pfalcon commented May 22, 2016

@jcmvbkbc : Thanks for confirming!

@pfalcon
Copy link
Owner

pfalcon commented May 22, 2016

Ok, the first patch of the series (switch to gcc 4.8.5) was merged. @Sermus, thanks for driving this. Please see update commit message and please follow them for future patches (well, at least for next patch to merge, as we'll keep processing them one by one). In the meantime, please rebase.

@dpgeorge: FYI

@pfalcon
Copy link
Owner

pfalcon commented May 22, 2016

One good way to upgrade to this patch is to "make clean" before pulling it, otherwise neither "make clean" won't work and will need fixing manually. This is also a good chance for a clean clone of esp-pen-sdk (all toolchain components will be redownloaded and rebuilt anyway).

@pfalcon
Copy link
Owner

pfalcon commented May 22, 2016

Actually, it requires git clean -d is crosstool-NG subdir.

Conflicts:
	.gitmodules
@Sermus
Copy link
Contributor Author

Sermus commented May 23, 2016

I merged the changes form upstream so that the pull request is no more in conflicted state with upstream.

@pfalcon
Copy link
Owner

pfalcon commented Jun 11, 2016

I still see that the next patch to merge in this patchset is "crosstool-NG was switched to xtensa-1.22.x. 1000-mforce-l32.patch was… …
Sermus committed on Mar 31
… retargetted to GCC 4.8.5", but that was already merged. If any progress is expected here, please follow (all) the suggestions above. Thanks.

@Sermus
Copy link
Contributor Author

Sermus commented Jun 14, 2016

I'll be off next two weeks, so there won't be progress on this, sorry.

@flannelhead
Copy link

flannelhead commented Mar 17, 2017

Is there any update on this? It would be really nice to have newlib fully usable in this SDK, which is my preferred choice for ESP8266 development. As of now, usage of some libc functions such as strtol results in linker errors about unknown references to _malloc_r etc.

Edit: What are the actual actions left to get this merged? I would like to help if they are something I'm able to do.

@romansavrulin
Copy link

@Sermus Very interested in this too. Is there any progress?

@Sermus
Copy link
Contributor Author

Sermus commented Mar 9, 2018

Sorry, Gents, I'm really busy with my current assignments. This lib is usable (you can check out in my fork of the esp-open-sdk). I'd appreciate if somebody could do a backmerge request to pfalcon's repo.

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.

None yet

8 participants