wstring: fix concatenation from flash

[d-a-v]

fixes #6367

[d-a-v]

@earlephilhower
while String s = FPSTR("s"); works, String s = PSTR("s"); has the same issue.

I'm not sure if operator= has to be overloaded or if ::copy(const char *cstr, should receive the same fix as for ::concat, or any other clever idea.

[earlephilhower]

The class structure in String gives me a headache, and I think the operator + helper classes could have the same kinds of issues.

@d-a-v, how about working around the issue by adding a "memmove_P" to the standard newlib functions instead? It's trivial:

void *memmove_P(void *dest, const void *src, size_t n) {
    if (src > (void*)0x40000000) return memcpy_P(dest, src, n);
    else return memmove(dest, src, n);
}

Yes, it crashes if you try to memmove with flash destination, but that's the correct behavior. :)

Something similar is being done for strlen() due to GCC having some silent optimizations that call it.

Then this fix becomes a sed s/memmove/memmove_P/g WString.cpp.

[everslick]

I don/t want to sound pushy or something, but can we have a intermittent fix for master in the meantime? ;-)

My firmware depends on += PSTR() in literally hundreds of places and I cannot push my current changes due to API changes in the FS layer.

I'm surprised nobody else complained about += being broken, yet.

[everslick]

Thanks!

[cr1st1p]

@earlephilhower
while String s = FPSTR("s"); works, String s = PSTR("s"); has the same issue.

I'm not sure if operator= has to be overloaded or if ::copy(const char *cstr, should receive the same fix as for ::concat, or any other clever idea.

Adding just PSTR() to a string and hoping things work is very bad. Never do this unless you really know what you do.
Read first https://arduino-esp8266.readthedocs.io/en/latest/PROGMEM.html

I commented on #6367 and I think you should actually revert this code change.

[d-a-v]

Adding just PSTR() to a string and hoping things work is very bad

It's not about hope&magic :)
There's only determinism and potential fixes for issues in open source world.

The fix can be done, but what @earlephilhower proposed in #6368 (comment) is better than what I proposed.
We just need to let him time to implement memmove_P and update strlen in libc (that he provides) then we can use it in WString.

[cr1st1p]

Adding just PSTR() to a string and hoping things work is very bad

It's not about hope&magic :)
There's only determinism and potential fixes for issues in open source world.

And ... the fix for the user's problem is for him to correctly use flash strings: to use F("sss") and NOT PSTR("xyz") .
Not changing some library code to make it work with a wrong use case, and automatically encouraging people believe that's a normal use of PSTR()
Especially since it looks like the flash strings usage is pretty good explained in https://arduino-esp8266.readthedocs.io/en/latest/PROGMEM.html

The fix can be done, but what @earlephilhower proposed in #6368 (comment) is better than what I proposed.
We just need to let him time to implement memmove_P and update strlen in libc (that he provides) then we can use it in WString.

@d-a-v : That fix is also using some strange hardcoded number (0x40000000).
But you're missing my point about wrong use and assumptions on PSTR() general use, versus use of PSTR() in WString concat. You are continuing to talk about WString use, while I'm describing a larger perspective of the issue.

Imagine people doing these:

• strtok(PSTR("some string"), ...)
• strpos(PSTR("some string")
• strcmp,strcasecmp(PSTR("somestring"), someVariable)
• gethostbyname(PSTR("google.com"))
• Library::function1(const char* c) : while(c && *c != '\0' && *c != ';') { do something }
  another library user: Library::function1(PSTR("some string"))

I hope you see now how bad it is to use PSTR() randomly and why promoting that kind of use is bad.

A better change - although it can also probably break things and might not be complete either, is to do changes in pgmspace.h:

• PGM_P -> cast to const __FlashStringHelper*
• PGM_VOID_P -> cast to either const __FlashStringHelper* or some other similar class (and make FlashStringHelper* derive from it)
• PSTR() -> add cast to PGM_P

Or just redirect people having problems to the docs I linked.

[d-a-v]

That fix is also using some strange hardcoded number (0x40000000).

This is specific to esp8266. All data above this address are "PROGMEM" and must be accessed by blocks of 32 bits. This code is supposed to be hidden in the newlib port of esp8266 (or cores/ subdirectory where you can see such constants too).

About the documentation on the arduino side, you are right and libraries need to use the correct macros when it comes to use PROGMEM data. We usually redirect users to the docs you mentionned when such issue occurs. Again, #6367 is about restoring something which was working in the past (I think, but anyway internally useful) and is not a good use case to show to users.

[cr1st1p]

@d-a-v Of course I figured out what's with the 0x40... value. But I would recommend creating a real constant in some header with it. Somehow I think it might be useful in more places.
Examples:

• gdbstub.c
• eboot.c

[d-a-v]

What about you create a pull request with your proposed recommended changes ?