bug with macros in ULP assembly files

[8785ben]

Hi
I'm using the esp32ulp assembler on .S files. I have been experiencing issues when using macro's.

esp32ulp-elf-as.exe does not produce valid .o files on macro's when processing the .pS file when it is big or a bit complex. The problems is that it will not increment correctly the instruction counter inside the macro.

For example, here is the code that fails (extract from the .d file):

Code: Select all

 
 114              	nrf24IsConnected:
 115 00b0 0000501A 	 readRegister 0x03
 115      01008072 
 115      35000072 
 115      01008072 
 115      0D000068 
 115      1F002072 
 115      00000080 
 115      F10F8072 
 115      01008072 
 115      0D000068 
 115      1F002072 
 115      00000080 
 115      0105501A 
 116 00e4 01000C82 	 jumpr nrf24IsNotConnected, 1, LT
 117 00e8 04000B82 	 jumpr nrf24IsNotConnected, 4, GE
 118 00ec 10008072 	 move r0, 1

where readRegister is a macro:

Code: Select all

.macro readRegister reg
	beginTransactionSpi
	move	r1,	R_REGISTER
	add		r1, r1, \reg
	
	move r1, ret1\@[/b]      // here we reference the address ret1
	st r1,r3,0 
	sub r3,r3,1
	jump SPI_Trans_Byte   // here we reference the address SPI_Trans_Byte
ret1\@:
	move	r1, 0xFF

	move r1, ret2\@           // here we reference the address ret2
	st r1,r3,0 
	sub r3,r3,1 
	jump SPI_Trans_Byte   // here we reference the address SPI_Trans_Byte
ret2\@:
	endTransactionSpi
.endm

You will notice that there are 4 places where I reference an address in the macro (in bold).

When dumping the relocation section in the .o file produced by the assembler, it can be noticed that all relocation information corresponding to the code inside the macro (lines 0xB0) are wrongly assigned to the same location (aka 0xB0), which is the address of the first instruction of the macro :

Code: Select all

$ esp32ulp-elf-readelf -r a.out

Section de r▒adressage '.rela.text' ▒ l'adresse de d▒calage 0xce8 contient 111 entr▒es:
 D▒calage   Info    Type            Val.-sym   Noms-symb + Addenda
00000024  00000105 R_ESP32ULP_ALUI   00000000   .text + 34
00000030  00002f01 R_ESP32ULP_RIMM16 00000000   SPI_Init + 0
00000050  00000105 R_ESP32ULP_ALUI   00000000   .text + 60
0000005c  00003101 R_ESP32ULP_RIMM16 00000000   SPI_Trans_Byte + 0
00000064  00000105 R_ESP32ULP_ALUI   00000000   .text + 74
00000070  00000105 R_ESP32ULP_ALUI   00000000   .text + 74
00000074  00003101 R_ESP32ULP_RIMM16 00000000   SPI_Trans_Byte + 0
00000090  00000105 R_ESP32ULP_ALUI   00000000   .text + a0
0000009c  00003101 R_ESP32ULP_RIMM16 00000000   SPI_Trans_Byte + 0
000000b0  00000105 R_ESP32ULP_ALUI   00000000   .text + cc                          <== address b0 is wrong
000000b0  00003101 R_ESP32ULP_RIMM16 00000000   SPI_Trans_Byte + 0          <== address b0 is wrong
000000b0  00000105 R_ESP32ULP_ALUI   00000000   .text + e0                          <== address b0 is wrong
000000b0  00003101 R_ESP32ULP_RIMM16 00000000   SPI_Trans_Byte + 0[/b]    <== address b0 is wrong
000000e4  0000010f R_ESP32ULP_JUMPR_ 00000000   .text + fc
000000e8  0000010f R_ESP32ULP_JUMPR_ 00000000   .text + fc
00000110  00000105 R_ESP32ULP_ALUI   00000000   .text + 12c
00000110  00003101 R_ESP32ULP_RIMM16 00000000   SPI_Trans_Byte + 0
00000110  00000105 R_ESP32ULP_ALUI   00000000   .text + 140
00000110  00003101 R_ESP32ULP_RIMM16 00000000   SPI_Trans_Byte + 0
00000148  00000105 R_ESP32ULP_ALUI   00000000   .text + 164
00000148  00003101 R_ESP32ULP_RIMM16 00000000   SPI_Trans_Byte + 0
00000148  00000105 R_ESP32ULP_ALUI   00000000   .text + 178
00000148  00003101 R_ESP32ULP_RIMM16 00000000   SPI_Trans_Byte + 0
00000188  00000105 R_ESP32ULP_ALUI   00000000   .text + 1a4
00000188  00003101 R_ESP32ULP_RIMM16 00000000   SPI_Trans_Byte + 0

So when the ulp.elf is produced the relocations are applied to the wrong locations.

I can solve the issue in the source code of the binutils-esp32ulp (I have recompiled the source code but the issue is still there) but I have no idea where macros are handled. Any idea ?

I have tried other methods like replacing .macro by #define but runs into the same trouble.

[8785ben]

Replying to myself :

After hours of investigation in the source code of the binutils, I have found the guilty.
The problems is in the gas/listing.c file, function void listing_newline (char *ps)
It returns when it finds that the same line has already been handled (lines 330):

Code: Select all

  file = as_where (&line);
  if (ps == NULL)
    {
      if (line == last_line
	  && !(last_file && file && filename_cmp (file, last_file)))
	return;

In this case, it will not create a new fragment of code and will continue the existing fragment.

This is what happens when we process a macro : the macro is in general made of several lines and when the parser processes the instructions defined in the macro, we stay on the same line of the source file (of course we don't move)...
The big problem is that all relocations appearing in the macro will be handled as if they were located on the first instruction of the macro, which is incorrect, thus if you consider this dummy piece of code (this is just an explanation of the problem, the code does not do anything interesting):

Code: Select all

mymacro .macro
               jump  myroutine
               wait   10
              .endm
              .text
test:     mymacro
             halt
myroutine:
             halt

will work (just by luck!) but

Code: Select all

mymacro .macro
               wait   10
               jump  myroutine
              .endm
              .text
test:     mymacro
             halt
myroutine:
             halt
              

will not work because the relocation that the linker needs to apply to jump myroutine will be wrongly applied to the first instruction (wait 10!). The relocation information put by gas into the .o file are incorrect and will make esp32ulp-elf-ld.exe do stupid things!
This explains why all macro's having a relocation (typically a jump) after the first ULP instruction will fail.

I have commented the if .. return statement and things now seem to be ok but I need to understand if there are side-effects.

Not sure to understand yet if this is the intended behaviour of gas (since this is not part of the customization for the esp32) or we have some problems with the way we handle line numbers or any other issue.
I will need to try something similar with a regular gas on a well-known processor architecture like an i386.

[8785ben]

Actually the root cause is the way md_assemble() has been coded in the tc-esp32ulp.c

My solution above, which modifies listing.c, is a workaround that will force generate a "frag" (aka fragment of code) for each ULP instruction, thus it doesn't matter where we are in the fragment since a fragment can only contain one single instruction (so offset in the fragment is always 0).

The correct solution is to leave listing.c but update md_assemble()
In fact the original code nearly contains the solution (but was commented) and I also discovered that it contains another bug which is not harmful:

Code: Select all

void
md_assemble(char *line)
{
...
	while (insn)
	{
		if (insn->reloc && insn->exp->symbol)
		{
			size = 4;
			//DEBUG_TRACE("insn->reloc && insn->exp->symbol BFD_ARELOC_ESP32ULP_PUSH size =%i, insn->exp->value=%i, insn->pcrel=%i, insn->reloc=%i\n", size, (unsigned int)insn->exp->value, insn->pcrel, insn->reloc);

			//char *prev_toP = toP - 2;
			//fix_new(frag_now, (prev_toP - frag_now->fr_literal), size, insn->exp->symbol, insn->exp->value, insn->pcrel, insn->reloc);
			// were - shift from current word... 
			fix_new(frag_now, 0, size, insn->exp->symbol, insn->exp->value, insn->pcrel, insn->reloc);  <- THE BIG BUG IS HERE AS WE PASS ALWAYS 0 AS THE OFFSET OF THE INSTRUCTION IN THE FRAGMENT OF CODE (2nd parameter of fix_new())
		}
		else
		{
			//DEBUG_TRACE("md_number_to_chars insn->value =%08x\n", (unsigned int)insn->value);
			//md_number_to_chars(toP, insn->value, 2);
			//toP += 2;
			// TODO:DYA - this is main changes for 32 bit words!!!!! changes stop work
			md_number_to_chars(toP, insn->value, 4);
			toP += 4;
		}
		size = 4;
#ifdef DEBUG
		//DEBUG_TRACE(" reloc : value = %08x, pc=%08x, reloc=%08x BFD_RELOC_ESP32ULP_PLTPC = %08x\n", (unsigned int)insn->value, (unsigned int)insn->pcrel, (unsigned int)insn->reloc, BFD_RELOC_ESP32ULP_PLTPC);
		if (insn->exp != ((void*)(0)))
		{
			//DEBUG_TRACE(" exp: %08x, sy_obj=%i\n", insn->exp->value, insn->exp->symbol->sy_obj.local);
		}
#endif
		insn = insn->next;
	}

Shall be changed to:

Code: Select all

void
md_assemble(char *line)
{
	char *toP = 0;
	int  insn_size;
	size_t len;
	static size_t buffer_len = 0;
	static char *current_inputline;
	parse_state state;

	len = strlen(line);
	if (len + 2 > buffer_len)
	{
		buffer_len = len + 40;
		current_inputline = XRESIZEVEC(char, current_inputline, buffer_len);
	}
	memcpy(current_inputline, line, len);
	current_inputline[len] = ';';
	current_inputline[len + 1] = '\0';

	state = parse(current_inputline);
	if (state == NO_INSN_GENERATED)
	{printf("no insn generated\n");	return;}

	// TODO:DYA insn_size - must be 4 in any case
	insn_size = 4;
	toP = frag_more(insn_size);
	last_insn_size = insn_size;

#ifdef DEBUG
	printf("INS: %s\n", line);
#endif
	int count = 0;
	while (insn)
	{
		if (insn->reloc && insn->exp->symbol)
		{
			// one of the "instruction" output by the parser contains a relocation on a symbol
			// we must indicate that the linker will have to do a relocation on this instruction
			// which is located at "toP" in the fragment of code (the instructions contained in this fragment starts at frag_now->fr_literal)
			fix_new(frag_now, toP-frag_now->fr_literal, size, insn->exp->symbol, insn->exp->value, insn->pcrel, insn->reloc);
			
		}
		else if (count == 0)
		{
			// the only real op-code that we need to put in the obj file is only the first one given by the parser
			// we dump it in the buffer (pointed by toP)
			// other "instructions" output by the parser are not real instructions (no op-code)
			md_number_to_chars(toP, insn->value, 4);
		}

#ifdef DEBUG
		//DEBUG_TRACE(" reloc : value = %08x, pc=%08x, reloc=%08x BFD_RELOC_ESP32ULP_PLTPC = %08x\n", (unsigned int)insn->value, (unsigned int)insn->pcrel, (unsigned int)insn->reloc, BFD_RELOC_ESP32ULP_PLTPC);
		if (insn->exp != ((void*)(0)))
		{
			//DEBUG_TRACE(" exp: %08x, sy_obj=%i\n", insn->exp->value, insn->exp->symbol->sy_obj.local);
		}
#endif

		insn = insn->next;
		count++;
	}

Now we compute correctly the offset of the instruction in the code fragment. This is also done this way in the implementations of a few other processor architectures I have looked at.

The other bug that I have discovered is that we tried to put all the pseudo-instructions given by the parser in the buffer, whereas only the first one contains the real instruction (with the op-code). The others are either empty or informations on relocation. This is not very harmful since they will be overriden when we will process the next real instruction but it could potentially lead to write to memory over the buffer (we reserve only 4 bytes at the begining of md_assemble when we do toP = frag_more(insn_size);

So I have decided to write in the buffer only the first instruction.

I will test my fix more deeply before proposing it to github. However now we shall be able to handle JUMP, JUMPR, JUMPS, MOV #IMM correctly in macros or when they are grouped with other instructions on a single line (separated by a ; )

[8785ben]

Problem solved and solution submitted to the project in github : https://github.com/espressif/binutils-esp32ulp/issues/9
I hope that one of the owner will make it available to community soon.

Correct implementation of md_assemble() in tc-esp32ulp.c:

Code: Select all

void
md_assemble(char *line)
{
	char *toP ;
	int  insn_size;
	size_t len;
	static size_t buffer_len = 0;
	static char *current_inputline;
	parse_state state;

	len = strlen(line);
	if (len + 2 > buffer_len)
	{
		buffer_len = len + 40;
		current_inputline = XRESIZEVEC(char, current_inputline, buffer_len);
	}
	memcpy(current_inputline, line, len);
	current_inputline[len] = ';';
	current_inputline[len + 1] = '\0';

	// run the parser on the instruction
	//   it will return a list of chained "insn",
	// 	 the first contains the opcode of the instruction
	//   and may be followed by other "insn" like an address
	state = parse(current_inputline);
	if (state == NO_INSN_GENERATED || !insn)
		return;

	// add 4 bytes to the fragment code buffer to put the new instruction
	// and get buffer pointer (toP) on where to write the instruction
	insn_size = 4;
	toP = frag_more(insn_size);			

#ifdef DEBUG
	printf("INS: %s\n", line);
#endif
	md_number_to_chars(toP, insn->value, insn_size);	// put the 4-byte instruction into the current fragment code buffer 
	
	while (insn)
	{
		if (insn->reloc && insn->exp->symbol)
		{
			// generate a relocation request for this instruction so that linker will put the right address
			//   toP is the pointer on this instruction in the buffer of the current code fragment 
			//   frag_now->fr_literal is the pointer on the begining of the buffer of the current code fragment
			fix_new(frag_now, toP - frag_now->fr_literal, insn_size, insn->exp->symbol, insn->exp->value, insn->pcrel, insn->reloc);
		}
		insn = insn->next;
	}
#ifdef OBJ_ELF
	dwarf2_emit_insn(insn_size);
#endif

	while (*line++ != '\0')
		if (*line == '\n')
			bump_line_counters();
}

For those interested I have also submitted an implementation for the ULP disassembler (esp32ulp-elf-objdump.exe) : https://github.com/espressif/binutils-e ... /issues/10

[ESP_Sprite]

Thanks so much for squirreling out this bug and finding the fix! We'll most certainly be looking at your patches and integrating them into the toolchain.

[8785ben]

You are welcome I really appreciate the ESP32 and the fact that you have published the source code of all the tools. Not always easy to understand every piece of code but it's really great

Let me know if there is anything you don't understand in what I have proposed.