Skip to content

fix umm memory managment #1630

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

Closed
wants to merge 3 commits into from
Closed

fix umm memory managment #1630

wants to merge 3 commits into from

Conversation

Links2004
Copy link
Collaborator

wrap all SDK memory functions in all libs, to get umm to work
fix umm_malloc.c:1708: undefined reference to `putchar'

@Links2004
Copy link
Collaborator Author

will fix #1629 and problems from #1586

@igrr
Copy link
Member

igrr commented Feb 14, 2016

pvPortMalloc and friends are already defined here: https://github.com/esp8266/Arduino/blob/master/cores/esp8266/heap.c, and corresponding functions from libmain.a are marked as weak, so is additional wrapping required?

@igrr
Copy link
Member

igrr commented Feb 14, 2016

Regarding printf: sorry for breaking the build, but let's fix the real issue (printf not compiling) instead of replacing it with ets_printf.

@me-no-dev
Copy link
Collaborator

I use umm without any wrapping at all. Have just the Port stuff weakened and overwritten. All has been working nice since some time before you even introduced it in the main repo. Have additionally added alignment check and put all in RAM and that's about it. :) 2 bytes poison and integrity check are on

@Links2004
Copy link
Collaborator Author

yes the wrapping is needed some of my projects only boot when the wrapping is here.
and all mem_* stuff is normally in the rom of the chip, for this we need wrapping too.

@Links2004
Copy link
Collaborator Author

to get printf work it is normally not more then this:

int putchar_cpp(int c) {
    if(Serial.write(c)){
        return c;
    }
    return -1;
}

extern "C" {
int putchar(int c) {
    return putchar_cpp(int c);
}
}

but i think debug an error messages are better in os_printf since it easy to disable the output when serial is needed for something else, and we have already many debug out at os_printf.
and it is better when the user can select where printf is printing.

@igrr
Copy link
Member

igrr commented Feb 14, 2016

Okay, figured that out. The issue has actually been triggered by 737f6c2, where i have replaced calls to os_malloc and os_free with malloc and free.

TL;DR: Depending on the libraries used by the sketch, weakening pvPortMalloc/vPortFree may be not sufficient.

Here is a simple sketch which will fail to run (stripped down version of the one provided by @Links2004):

#include <Arduino.h>
#include <ESP8266WiFi.h>

void setup() {
    Serial.begin(115200);
    Serial.setDebugOutput(true);
    Serial.println();
    Serial.println();
    Serial.println();
    for(uint8_t t = 4; t > 0; t--) {
        Serial.printf("[SETUP] BOOT WAIT %d...\n", t);
        delay(1000);
    }
    Serial.printf("[SETUP] HEAP: %d\n", ESP.getFreeHeap());
    WiFi.mode(WIFI_AP);
    WiFi.softAP("ESP_SLDT", "yesSecureHere");
}

void loop() { }

At the same time, more complex sketches (e.g. FSBrowser sample) seem to work fine.
The reason this happens has to do with linker search order. If none of the object files before libmain.a reference pvPortMalloc symbol, it will not be pulled from arduino.ar (archive file containing the core library). Instead, linker will loop inside libmain.a until pvPortMalloc/vPortFree are found (that's the rule for dealing with archive files: loop until no additional symbols are resolved). If the sketch uses a library which calls pvPortMalloc (usually this happens via os_malloc and os_free defines), for instance ESP8266mDNS, linker will process it first, and will resolve the reference to pvPortMalloc via arduino.ar, pulling the "right" definition. From that moment on, the right definition will be used elsewhere.

I'm going to remove mem_manager.o from libmain.a completely, so that the "right" symbols are always used.

igrr added a commit that referenced this pull request Feb 14, 2016
@igrr igrr closed this Feb 15, 2016
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