Maximum patch size?

#21

i agree, it is a bit surprising that this has not been hit more. However, it seems that what's in this section in practice is a pointer to a list of function pointers, which thus corresponds to at most two instructions.

Another way of looking at it is that if the problem had caused more problems, it would have been fixed earlier as it would have been more obvious that something waas wrong. By sheer luck, the instructions resulting from the pointers have in most cases been harmless, if that hadn't been the case, the system would have crashed and misbehaved so often that someone would have needed to have a lok at it a long time ago.

To me, it's quite obvious that the constructor/destructor arrays should not be called as code ... and it wasn't always that way, those sections were introduced in commit
203580f5... on 2015-08-04, thus it wasn't part of the original design. The constructor list is explicitly traversed in xpatch_init2(), so there's no way it can be meant to be executed directly.

It all ties in with the symptoms too: the dependence on the patch size, especially that it is not a linear correspondance, i.e. patches don't necessarily break over a certain size, the relative independence of the firmware code, the fact that the constructor code in my case equates to setting r4 to 1 which is the behavior I've observed, not to mention the disassembly of the code actually executed when calling PATCHMAINLOC.

Anyhow, I tested my 'problem patch' with the constructor and destructor sections moved to after the text section in xpatch.bin, and it works fine now, in all configurations of it (I have been testing by remove one, two or three calls to my printpar() local function, with the the constructors list at the start of xpatch.bin, removing all three has always worked, and depending on the rest of the code removing one or two has sometimes worked, wheras leaving all three there has never worked.)

#22

oops, my bad :frowning:

was quite some time back... I did it to fix issue with static dtor/ctor not being called... which worked,
and I placed it in the same 'relative' place as is done in the STM32.ld file...
but it didn't occur to me that of course, the patch start point is consider to be at the beginning...

I'll move it now...

BUT...
I suspect the correct implementation is to change .text to ...

.text : ALIGN(16) SUBALIGN(16)
{
    PROVIDE(__code_start = .);
    * (.boot);
    * (.text);
    etext = .;
} > SRAM

such that the start of the code, is not hardcoded but derived from the ld file...
(so if it moves , changes, then this will move with it)

thoughts?

EDIT: forgot to say very cool bit of debugging... thanks for putting the effort into this :boom:

#23

I'm assuming you mean in addition to moving the constructors/destructors segment? Otherwise there will still be an offset from the start of the binary to the xpatch_init() function.

I must say I'm not sure what PROVIDE(__code_start = .); actually does. Does it guarantee that the start of the code is at the start of the .text segment? Or is there some form of attribute one can give to a specific function so that the linker puts it at the address specified by __code_start?

Actually, looking at xpatch.cpp, this is actually done already: the code for xpatch_init() is as follows (from xpatch.cpp):

extern "C" __attribute__ ((section(".boot"))) void xpatch_init(int fwid){
  xpatch_init2(fwid);
}

and the __attribute__((section(".boot")) puts it in the .boot section which is before the .text section in the .text segment. But maybe I'm misunderstaning you as I don't really know what PROVIDES really does.

Thanks. I just couldn't walk away from this (and I figured if I couldn't fix it I wouldn't be able to get any further with the patch I was working on...).

#24

provide gives you a variable initialised to the actually position allocated by the linker.

yes, i meant in addition.

but I realised, it doesn't actually help... as the pointer is needed in patch.c, not in xpatch.
I guess something could be defined in STM32F407.ld, but it doesnt really help, as its just having to trust that the binary sent, really does have the code at the beginning.
(the section code, you show above forces this, assuming .boot is at the start)

still amazed, we haven't seen more issues... guess the wind has been blowing in the right direction.

#25

Ah. That simple. I should have guessed...

In ramlink.ld, .boot is before .text, so yes. Perhaps there should be a comment there that it really needs to be first so no one misses it in the fuure.

The funny thing is that I have had various issues, i.e. things misbehaving, but I always assumed I'd done something wrong in the patch so I changed someting and voila, it would work again. So probably there have been a few hickups which people have passed off as other errors.

#26

A quick note to anyone reading this thread: as noted in another thread

a fix for this has now been merged to the Axoloti firmware master branch.