-
Notifications
You must be signed in to change notification settings - Fork 13.3k
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
Allow non-aligned PSTR() #7275
Allow non-aligned PSTR() #7275
Conversation
That's one hell of an ugly command line Can you also please put in the same PR to https://github.com/earlephilhower/newlib-xtensa ? The newlib-xtensa is the original source of this header, and when we update the library or compiler, if it's not in that other repo changes will get lost. |
I counted 1128 PSTRs in Tasmota. And I agree this is the ugliest command line option I've ever seen. I maybe escaped too much, but I won't optimize it. I will do the PR in newlib-xtensa too. |
Can we add a Edited: changed reference to 16 to 4-byte alignment. |
@mhightower83 are you sure about the 16 byte alignment thing? I am pretty sure we never had a 16 byte aligned PSTR. It used to be packed, byte-wise (which is what this PR is enabling)... |
Oops, brain fog today that should have been 4 bytes. PSTR4() |
@s-hadinger as @mhightower83 said, there may be issues with changing the alignment. How much have you tested with that macro override? |
I didn't think this could be an issue since regular strings are not 4-bytes aligned. I have Tasmota devices running for a few days with this macro and didn't observe any issue or crash. There was no issue reported on Discord either, but it's still a recent change. |
I would expect a possible problem when |
-edit- I see we crossposted, but had the same answer. :) @s-hadinger, it's a little more complicated than that. RAM is fine reading byte-wide, but IROM(PSTR) needs special instructions to only to 32-bit reads and extract the byte needed. The main library routines (all printf* variants, strcpy, etc.) all work with any alignment of data in strings. However, @mhightower83 has some routines in a special-use section that need to be 4 byte aligned because his code can't afford to do the adjustments. |
Good to know. As far as I know, we don't compile with The only places where I found a use of Did I miss something? Anyways, there is no harm in adding a |
@s-hadinger, sorry there is a bit of macro nesting going on here to make it easier to change things as a group:
and I think only that last line will need to be changed to use |
@mhightower83 thanks, I missed that macro. @earlephilhower I changed the PR to add |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thx. We should get @mhightower83 to chime in, too.
Thanks! LGTM. |
Gentle bump. Is there anything more needed for merging? |
@s-hadinger can you also port these final changes to newlib so they don't get lost? Thx! |
Reporting changes from esp8266/Arduino#7275
With either 1 or 4 byte alignment I am getting crashes on my own code, which uses PSTR in the way tasmota does. To be specific "loadProhibited" and errors related to alignment when trying to use debug builds with vscode/platformio. Did you come across this and if so did you manage to clear the error. Many thanks. |
@sparkplug23 Can you share the code of where it crashes? |
@s-hadinger Thanks for replying. After spending days on this, as it always happens, the second you ask for help you find the problem! It seems I was using a PSTR incorrectly with |
PSTR()
are by default aligned to 32-bits boundaries, whereas normal string are not. I understand the speed benefit, but it consumes some additional Flash, which is around 1.7KB for Tasmota.This change allows to override this default behavior and allow non-aligned PSTR() with
#define PSTR_ALIGN 1
.There is no change if you don't explicitly set this flag as compile option
-DPSTR_ALIGN=1
.Currently the only way in Tasmota to achieve the same result is to override the entire
PSTR()
macro with:-DPSTR\(s\)=\(__extension__\(\{static\ const\ char\ __c\[\]\ __attribute__\(\(__aligned__\(1\)\)\)\ __attribute__\(\(section\(\ \"\\\\\".irom0.pstr.\"\ __FILE__\ \".\"\ __STRINGIZE\(__LINE__\)\ \".\"\ \ __STRINGIZE\(__COUNTER__\)\ \"\\\\\"\,\ \\\\\"aSM\\\\\"\,\ \@progbits\,\ 1\ \#\"\)\)\)\ =\ \(s\)\;\ \&__c\[0\]\;\}\)\