String += PSTR() crashes (and it should on Arduino, but can esp8266 be more flexible?)

[everslick]

Basic Infos

• This issue complies with the issue POLICY doc.
• I have read the documentation at readthedocs and the issue is not addressed there.
• I have tested that the issue is present in current master branch (aka latest git).
• I have searched the issue tracker for a similar issue.
• If there is a stack dump, I have decoded it.
• I have filled out all fields below.

Platform

• Hardware: [ESP-12]
• Core Version: [d6973cd]
• Development Env: [Make]
• Operating System: [Ubuntu]

Settings in IDE

• Module: [Generic ESP8266 Module]
• Flash Mode: [dio]
• Flash Size: [4MB]
• lwip Variant: [v2 Lower Memory]
• Reset Method: [ck]
• Flash Frequency: [40Mhz]
• CPU Frequency: [160MHz]
• Upload Using: [SERIAL]
• Upload Speed: [115200)

Problem Description

Exception 3 when I concatenate a global const char * that is declared to be stored in flash via the PSTR() macro to a String object.

Last known good git version is: 0ab76fc from 2019/05/09

MCVE Sketch

#include "Arduino.h"

static const char *gpstr;
static String gstr = "foo";

void setup(void) {
  Serial.begin(115200);

  Serial.println("Start");
  gpstr = PSTR("string stored in flash");

  Serial.println("Before Crash");
  gstr += gpstr; // << crash
  Serial.println("Not reached");
}

Debug Messages

ets Jan  8 2013,rst cause:2, boot mode:(3,6)                                            
                                                                                         
load 0x4010f000, len 1384, room 16                                                       
tail 8                                                                                   
chksum 0x2d                                                                              
csum 0x2d                                                                                
v00000000                                                                                
~ld                                                                                      
Start                                                                                 
Before Crash                                                                             
                                                                                         
Exception (3):                                                                           
epc1=0x40235680 epc2=0x00000000 epc3=0x00000000 excvaddr=0x4027be94 depc=0x
00000000                                                                                 
>>>stack>>>                                                                      
ctx: cont                                                             
sp: 3fff1250 end: 3fff1460 offset: 01a0

Stacktrace

clemens@secrets:~/Devel/ESP/emonio-fw/src$ make stack 
0x3ffefdd0: ?? at /home/clemens/Devel/ESP/emonio-fw/src/emonio.ino:4
0x40202331: Print::write(char const*) at /home/clemens/Devel/ESP/emonio-fw/src/../../Arduino-ESP8266/cores/esp8266/Print.h:60
0x3ffefdd0: ?? at /home/clemens/Devel/ESP/emonio-fw/src/emonio.ino:4
0x40229864: String::concat(char const*) at /home/clemens/Devel/ESP/emonio-fw/src/../../Arduino-ESP8266/cores/esp8266/WString.cpp:342
0x4027be94: umm_block_size at ??:?
0x3ffefdd0: ?? at /home/clemens/Devel/ESP/emonio-fw/src/emonio.ino:4
0x402282bc: Print::println(char const*) at /home/clemens/Devel/ESP/emonio-fw/src/../../Arduino-ESP8266/cores/esp8266/Print.cpp:190
0x3ffefdd0: ?? at /home/clemens/Devel/ESP/emonio-fw/src/emonio.ino:4
0x402020c0: setup at /home/clemens/Devel/ESP/emonio-fw/src/emonio.ino:25
0x402011f0: loop_wrapper() at /home/clemens/Devel/ESP/emonio-fw/src/../../Arduino-ESP8266/cores/esp8266/core_esp8266_main.cpp:137
0x3ffe8618: ?? at /home/clemens/Devel/ESP/emonio-fw/src/../../Arduino-ESP8266/cores/esp8266/core_esp8266_main.cpp:55
0x40100221: cont_wrapper at /home/clemens/Devel/ESP/emonio-fw/src/../../Arduino-ESP8266/cores/esp8266/cont.S:81

[everslick]

d-a-v@35e0ad2 works for me!

[cr1st1p]

@d-a-v , @earlephilhower
Just got my coffee, but somehow I think there is something bogus in here:
PSTR just moves your string into progmem. It does not change the string's declared type (char )
As such, I would expect that most functions using a PSTR() string given as 'char' can fail, unless they're using _P functions in their implementations.
Think of user calling functions like strcat, strcpy, memmove, memcpy, String() functions, etc. I doubt they were all made to cope with progmem strings as well.

As an example, related to user's code, even a code like String gstr(gpstr) would crash (untested).
My version of WString.cpp on disk does nothing particular in the String::copy(const char* cstr, ...) function, used in the constructor. It uses standard memmove()

To me, it looks like if you want to use strings stored in flash, always use F() and FPSTR() and not just PSTR().

[cr1st1p]

To continue:
@d-a-v , @earlephilhower :
I would actually revert your fix:
a) I think it is incomplete anyway (constructor is probably having the same crash behavior)
b) it might have negative speed impact (for non-flash strings)
b) and it is unfortunately encouraging people to wrongly use flash stored strings.
You should point them to documentation, all of them which must state that you should NOT continue to use a flash stored string as you were using it before.

@everslick You should stop using PSTR() strings in such a way, unless you really know what you do (like use the PSTR() string into a _P function call).
You can bump into other crashes as well. Imagine a library that you use and wants a string (char*) parameter. If you use a PSTR() string... big chance library code crashes.
Really, using flash stored strings is not only about just adding PSTR() to your string...

Before using flash stored strings, please ensure you read documentation:
https://arduino-esp8266.readthedocs.io/en/latest/PROGMEM.html (best!)
https://www.arduino.cc/reference/en/language/variables/utilities/progmem/
Simplest and safest change would be to use the F() macro.

[d-a-v]

Per #6368 (comment),
This arduino core is compatible with the reference you mention,
but we will propose the ability to use progmem strings without need for the usual helpers/macro.

[cr1st1p]

@d-a-v
Is there a proposal somewhere regarding no use of helpers/macros? I would be interested to check it.
Meanwhile, maybe you can do a check of the examples given in #6368 (comment)
and let me know if your proposal will cover those cases as well.
Thank you!

[d-a-v]

Is there a proposal somewhere regarding no use of helpers/macros?

None that I'm aware of.

It is true in #6368 (comment) that @everslick should have used

F("sss") and NOT PSTR("xyz") .

This issue and the proposed temporary merged PR is only happening to restore something that was working before some commit, on esp8266 arduino only, even if the API is not correctly used. All that because there were some optimizations in newlib to internally and transparently allow the use of data from flash without need of copying them to ram before use.

[bxparks]

I'm a bit confused about this. I thought the expression String += PSTR() is supposed to crash according to the semantics of the Arduino API. It seems like ESP8266 has a hack to allow it to work, but that code is incompatible with AVR. My concern is that this convenience hack will encourage people to ignore the difference between an F() and PSTR(), and they will proliferate non-portable code.

[d-a-v]

@bxparks you are right.
Long answer there: #6368 (comment)