Closed earlephilhower closed 4 years ago
As the register
keyword is deprecated and, in C++17, effectively banned, perhaps it should be removed? Unless you're aware of a specific use for it within the xtensa toolchain?
http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2015/n4340
EDIT: Although the keyword still appears in the C standards I suspect it doesn't really do anything any more. But I could be wrong!
https://stackoverflow.com/questions/37456494/why-was-the-register-keyword-created
No, I think you're right. Or, at least, C++17 makes it a warning deprecated
and it breaks CI. I remember having to fix this in our Arduino core.
Although I'm not exactly sure what register
you're referring to. Can you provide a link?
I'll also make asm->asm or whatever is the "right" way to make that happen, as you commented in the Sming repo IIRC.
https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html
The asm keyword is a GNU extension. When writing code that can be compiled with -ansi and the various -std options, use
__asm__
instead of asm (see Alternate Keywords).
Although I'm not exactly sure what
register
you're referring to. Can you provide a link?
It's used extensively within Arduino, but there are two instances within pgmspace.h
in pgm_read_byte_inlined()
and pgm_read_word_inlined()
:
static inline uint8_t pgm_read_byte_inlined(const void* addr) {
register uint32_t res;
pgm_read_with_offset(addr, res);
return (uint8_t) res; /* This masks the lower byte from the returned word */
}
I've just attempted a Linux compile with optimisations disabled, and got this:
In file included from /home/mike/sandboxes/sming-dev/Sming/Arch/Esp8266/Components/libc/include/sys/pgmspace.h:43,
from /home/mike/sandboxes/sming-dev/Sming/Wiring/FakePgmSpace.h:14,
from /home/mike/sandboxes/sming-dev/Sming/Wiring/FakePgmSpace.cpp:11:
/opt/esp-quick-toolchain/xtensa-lx106-elf/xtensa-lx106-elf/include/sys/pgmspace.h: In function 'uint32_t pgm_read_dword_unaligned(const void*)':
/opt/esp-quick-toolchain/xtensa-lx106-elf/xtensa-lx106-elf/include/sys/pgmspace.h:97:1: error: a15 cannot be used in asm here
97 | }
| ^
It's objecting to the A15 clobber, if I remove that it's fine.
(Note: I only ran this test at -O0
to check if libstdc++
got implicated, but thought I should mention it.)
Got it on the format changes, thanks. I guess since I didn't include the right pgmspace.h they didn't pop up for me. :)
For a15
thing is probably a clobber list problem for me. I'll take a look at it. We never use -O0
because code blow up so bad, but I can see how it would make GDB much nicer...
@earlephilhower Did you forget to change asm
-> __asm__
as per https://github.com/earlephilhower/newlib-xtensa/issues/3#issuecomment-561403245 ?
I did try the code with gcc9.2 and std=c++17 and -O0 in arduino with no errors. Are you sure about having an issue with asm(xxxx) as opposed to the 1-line asm ?
This is the error I get:
/home/mike/sandboxes/esp-quick-toolchain/xtensa-lx106-elf.x86_64/xtensa-lx106-elf/include/sys/pgmspace.h:72:3: error: expected ')' before ':' token
72 | pgm_read_with_offset(addr, res);
| ^~~~~~~~~~~~~~~~~~~~
Can you give a small .C file I can compile locally, and any GCC flags? I'm not seeing that (and it seems to be building in the Arduino CI OK, too)...
#include <sys/pgmspace.h>
int main()
{
auto c = pgm_read_byte(PSTR("abc"));
return 0;
}
Compile with -std=c11
(or c99, etc) and it fails.
If you build with g++
it doesn't mind... suspect that's an oversight in the compiler.
Forgot to say, use gcc
and it fails.
This is interesting: https://stackoverflow.com/questions/13870489/is-inline-asm-part-of-the-ansi-c-standard. I don't have a problem working around this issue if it causes you problems. It only crops up compiling SPIFFS - we use the original source but I note arduino has pulled it in and converted to .cpp which is why this issue never comes up.
I don't mind adding underscores, but I really can't repro this on my own setup even with the example you gave. I'm running from inside the esp-quick-toolchain dir and here's what I get. I think there's something else going on is my worry:
earle@server:~/src/esp-quick-toolchain$ cat a.c
#include <sys/pgmspace.h>
int main()
{
auto c = pgm_read_byte(PSTR("abc"));
return 0;
}
earle@server:~/src/esp-quick-toolchain$ cp a.c a.cpp
earle@server:~/src/esp-quick-toolchain$ ./xtensa-lx106-elf.x86_64/bin/xtensa-lx106-elf-g++ -c --std=c++11 a.cpp
earle@server:~/src/esp-quick-toolchain$ ./xtensa-lx106-elf.x86_64/bin/xtensa-lx106-elf-g++ -c --std=c++11 a.c
earle@server:~/src/esp-quick-toolchain$ ./xtensa-lx106-elf.x86_64/bin/xtensa-lx106-elf-g++ -c --std=c++17 a.c
earle@server:~/src/esp-quick-toolchain$ ./xtensa-lx106-elf.x86_64/bin/xtensa-lx106-elf-g++ -c --std=c++17 a.cpp
earle@server:~/src/esp-quick-toolchain$ ./xtensa-lx106-elf.x86_64/bin/xtensa-lx106-elf-gcc -c --std=c++17 a.cpp
earle@server:~/src/esp-quick-toolchain$ ./xtensa-lx106-elf.x86_64/bin/xtensa-lx106-elf-gcc -c --std=c++17 a.c
cc1: warning: command line option '-std=c++17' is valid for C++/ObjC++ but not for C
a.c: In function 'main':
a.c:5:8: warning: type defaults to 'int' in declaration of 'c' [-Wimplicit-int]
5 | auto c = pgm_read_byte(PSTR("abc"));
| ^
earle@server:~/src/esp-quick-toolchain$
To be clear, this is the latest -gnu4 build (with all the PRs included). I just got home and will do the upload now.
Ignore the auto
thing, I'm too used to working in C++. See my addendum about this only failing with gcc
, i.e:
mike@UbuntuME:~/sandboxes/sming-dev/tests/asmtest$ /opt/esp-quick-toolchain/xtensa-lx106-elf/bin/xtensa-lx106-elf-gcc -c test.c -std=c11
In file included from test.c:2:
/home/mike/sandboxes/esp-quick-toolchain/xtensa-lx106-elf.x86_64/xtensa-lx106-elf/include/sys/pgmspace.h: In function 'pgm_read_byte_inlined':
/home/mike/sandboxes/esp-quick-toolchain/xtensa-lx106-elf.x86_64/xtensa-lx106-elf/include/sys/pgmspace.h:72:3: error: expected ')' before ':' token
72 | pgm_read_with_offset(addr, res);
| ^~~~~~~~~~~~~~~~~~~~
Ah, got it. -std=c11
not --std=c++11
. I can repo the failure now.
Newlib is fixed and I'm rebuilding the whole kaboodle. Another hour or so and -gnu4 should be up.
https://github.com/esp8266/Arduino/pull/6565