Skip to content
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.

Sign up for GitHub

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

Use 32b loads to set print strings #7545

Merged
merged 4 commits into from
Oct 16, 2020
Merged

Use 32b loads to set print strings #7545

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
46b221d
Use 32b loads to set print strings
earlephilhower Aug 23, 2020
88c9452
Merge branch 'master' into bootmin
earlephilhower Sep 2, 2020
268f29a
Merge branch 'master' of https://github.com/esp8266/Arduino into bootmin
earlephilhower Oct 15, 2020
14c5303
Merge branch 'master' into bootmin
earlephilhower Oct 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 23 additions & 14 deletions bootloaders/eboot/eboot.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,20 +21,20 @@ extern unsigned char _gzip_dict;
extern void ets_wdt_enable(void);
extern void ets_wdt_disable(void);

// Converts bit of a string into a uint32
#define S(a,b,c,d) ( (((uint32_t)a) & 0xff) | (((uint32_t)b) << 8) | (((uint32_t)c) << 16) | (((uint32_t)d)<<24) )

int print_version(const uint32_t flash_addr)
{
uint32_t ver;
if (SPIRead(flash_addr + APP_START_OFFSET + sizeof(image_header_t) + sizeof(section_header_t), &ver, sizeof(ver))) {
return 1;
}
char fmt[7];
fmt[0] = 'v';
fmt[1] = '%';
fmt[2] = '0';
fmt[3] = '8';
fmt[4] = 'x';
fmt[5] = '\n';
fmt[6] = 0;
// We don't have BSS and can't print from flash, so build up string
// 4 chars at a time. Smaller code than byte-wise assignment.
uint32_t fmt[2];
fmt[0] = S('v', '%', '0', '8');
fmt[1] = S('x', '\n', 0, 0);
ets_printf((const char*) fmt, ver);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more readable solution I guess is:

#define TINYSTR(str) [__builtin_strlen(str) + 1] = str
#define TINYCHARS(chars) [__builtin_strlen(chars)] = chars

and then,

    const char fmt TINYSTR("v%08x\n");
    ets_printf(fmt, ver);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OMG, greater then 4 bytes string literals will be placed into RODATA (and memcpy will copy them)...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry,

#define S(str) ((uint32_t)(str"\0\0\0")[0] | ((uint32_t)(str"\0\0\0")[1] << 8) | ((uint32_t)(str"\0\0\0")[2] << 16) | ((uint32_t)(str"\0\0\0")[3] << 24))

and

    const uint32_t fmt[] = { S("v%08"), S("x\n") };
    ets_printf(fmt, ver);

are OK (now placed into .text).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there aren't any really good solutions. We don't allow RODATA because we don't have the C init stuff in the bootloader. Can't use PSTR because ets_printf can't handle PROGMEM byte accesses. GCC's multichar literal puts things in the wrong order for strings (endian issue). So we end up with either a 4-param or 1-param marco w/array indexing.

Honestly, I like the existing setup more than the string 4-chars because with the current macro, if you accidentally put in a >4 char element it will error (while the string[index] will silently allow it).

But, your idea to make it a const array to get in .text, that's pretty cool. Let me see what it does to the size, might buy 4-8 bytes per instance...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Argh, looks like that's not going to work. I see .rodata appear when I do const uint32_t s={S(),S()}; Back where we were...

Copy link
Collaborator Author

@earlephilhower earlephilhower Aug 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, anyway, the string needs to be in RAM since it will be byte accessed. So if it was reduced to just a pointer to ICACHE (where eboot lives)/PROGMEM it would end up SEGV'ing. Has to be a copy to RAM step no matter what.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha, get to be quite weird definition...
( #include <assert.h> needed )

#define S(x, str) \
    do { \
        const unsigned int l = (__builtin_strlen(str) + 1 + 3) / 4; \
        static_assert(sizeof(x) / sizeof(*x) == l); \
        unsigned int i = 0; \
        while(i < l) \
            x[i] = ((uint32_t)(str"\0\0\0")[i * 4 + 0] | ((uint32_t)(str"\0\0\0")[i * 4 + 1] << 8) | ((uint32_t)(str"\0\0\0")[i * 4 + 2] << 16) | ((uint32_t)(str"\0\0\0")[i * 4 + 3] << 24)), ++i; \
    } while(0)

and

void test(void) {
    uint32_t fmt[3];
    S(fmt, "foo bar baz");

    extern int write(const void*);
    write((const char*)fmt);
}

emits the below (good result)

	.file	"test.c"
	.text
	.literal_position
	.literal .LC0, 544173926
	.literal .LC1, 544366946
	.literal .LC2, 8020322
	.align	4
	.global	test
	.type	test, @function
test:
	l32r	a2, .LC0
	addi	sp, sp, -32
	s32i.n	a2, sp, 0
	l32r	a2, .LC1
	s32i.n	a0, sp, 28
	s32i.n	a2, sp, 4
	l32r	a2, .LC2
	s32i.n	a2, sp, 8
	mov.n	a2, sp
	call0	write
	l32i.n	a0, sp, 28
	addi	sp, sp, 32
	ret.n
	.size	test, .-test
	.ident	"GCC: (GNU) 10.1.0"

but this

void test(void) {
    uint32_t fmt[4];
    S(fmt, "foo bar baz ");  /* 1 whitespace appended */

    extern int write(const void*);
    write((const char*)fmt);
}

emits bad...

	.file	"test.c"
	.text
	.section	.rodata
.LC0:
	.string	"foo bar baz "
	.string	""
	.string	""
	.string	""
	.text
	.literal_position
	.literal .LC1, .LC0
	.align	4
	.global	test
	.type	test, @function
test:
	addi	sp, sp, -32
	l32r	a3, .LC1
	s32i.n	a0, sp, 28
	movi.n	a4, 0
.L2:
	l8ui	a2, a3, 1
	l8ui	a5, a3, 2
	slli	a2, a2, 8
	slli	a5, a5, 16
	or	a2, a2, a5
	l8ui	a5, a3, 0
	add.n	a6, sp, a4
	or	a2, a2, a5
	l8ui	a5, a3, 3
	addi.n	a4, a4, 4
	slli	a5, a5, 24
	or	a2, a2, a5
	s32i.n	a2, a6, 0
	addi.n	a3, a3, 4
	bnei	a4, 16, .L2
	mov.n	a2, sp
	call0	write
	l32i.n	a0, sp, 28
	addi	sp, sp, 32
	ret.n
	.size	test, .-test
	.ident	"GCC: (GNU) 10.1.0"

return 0;
}
Expand Down Expand Up @@ -222,26 +222,32 @@ int main()
}

if (cmd.action == ACTION_COPY_RAW) {
ets_putc('c'); ets_putc('p'); ets_putc(':');
uint32_t cp = S('c', 'p', ':', 0);
ets_printf((const char *)cp);

ets_wdt_disable();
res = copy_raw(cmd.args[0], cmd.args[1], cmd.args[2], false);
ets_wdt_enable();

ets_putc('0'+res); ets_putc('\n');
cp = S('0' + res, '\n', 0, 0 );
ets_printf((const char *)cp);
#if 0
//devyte: this verify step below (cmp:) only works when the end of copy operation above does not overwrite the
//beginning of the image in the empty area, see #7458. Disabling for now.
//TODO: replace the below verify with hash type, crc, or similar.
// Verify the copy
ets_putc('c'); ets_putc('m'); ets_putc('p'); ets_putc(':');
uint32_t v[2];
v[0] = S('c', 'm', 'p', ':');
v[1] = 0;
ets_printf(const char *)v);
if (res == 0) {
ets_wdt_disable();
res = copy_raw(cmd.args[0], cmd.args[1], cmd.args[2], true);
ets_wdt_enable();
}

ets_putc('0'+res); ets_putc('\n');
cp = S('0' + res, '\n', 0, 0 );
ets_printf((const char *)cp);
#endif
if (res == 0) {
cmd.action = ACTION_LOAD_APP;
Expand All @@ -256,8 +262,11 @@ int main()
if (cmd.action == ACTION_LOAD_APP) {
ets_putc('l'); ets_putc('d'); ets_putc('\n');
res = load_app_from_flash_raw(cmd.args[0]);
//we will get to this only on load fail
ets_putc('e'); ets_putc(':'); ets_putc('0'+res); ets_putc('\n');
// We will get to this only on load fail
uint32_t e[2];
e[0] = S('e', ':', '0' + res, '\n' );
e[1] = 0;
ets_printf((const char*)e);
}

if (res) {
Expand Down
Binary file modified bootloaders/eboot/eboot.elf
View file
Open in desktop
Binary file not shown.