stlink
stlink copied to clipboard
stlink-org
Should we care about ABI and semantic versioning for stlink libs?
I see that at least 2 structures that expose over lib border changed its shape between ver 1.6.0 and 1.6.1 and still changes. It is struct stlink_t and struct stlink_chipid_params. So if we care about ABI we should change their shape in backwards-compatible form.
This is how struct stlink_chipid_params look like in version 1.6.0
struct stlink_chipid_params {
uint32_t chip_id;
char *description;
enum stlink_flash_type flash_type;
uint32_t flash_size_reg;
uint32_t flash_pagesize;
uint32_t sram_size;
uint32_t bootrom_base;
uint32_t bootrom_size;
};
This is how it looks now :
struct stlink_chipid_params {
uint32_t chip_id;
char *description;
enum stlink_flash_type flash_type;
bool has_dual_bank;
uint32_t flash_size_reg;
uint32_t flash_pagesize;
uint32_t sram_size;
uint32_t bootrom_base;
uint32_t bootrom_size;
uint32_t option_base;
uint32_t option_size;
};
As you can see there is bool has_dual_bank was added before flash_size_reg field. This is mean that in old structure flash_size_reg have offset 12 bytes. Now an offset for flash_size_reg 16. An old code doesn't know about these changes so it will misread flash_size_reg from new has_dual_bank field.
But, actually, I didn't dig deeper. So it is a question that we haven't similar breakage for previous releases.
So there is a question should we care about it in future?
I see what you mean. I believe this is also what you meant with the preference to allocate new parameters at the end of structures.
Maybe this link is somehow useful here: https://stackoverflow.com/questions/25947997/how-does-sizeofstruct-help-provide-abi-compatibility
I think the intermediate structure may help.
I believe this is also what you meant with the preference to allocate new parameters at the end of structures.
Yes, I have ABI in my mind when saying that. But this trick works only if in user code used pointers to structs. At first sight, it correct for our codebase.
@chenguokai Looks useful indeed. So, I guess it would make sense to add a note in docs, right?
I will think about it. I am not sure what it should be. Should it be some tests or just a guide on how to do it? Or a mix of tests and guide. But stable ABI is not a one-time issue. It is a process.
Well, I'd go through such spots where this topic could be relevant. Shall there be any findings that want improvement one could think about fixing it in place. Additionally some documentation in the guidelines seems reasonable. Anything else, especially future work should follow these then. Thus we shall see some continuous improvements.
I think, for the best, external programs should not use the structure directly. Needs to revise the contents of the include files. External applications should only be given the stlink_* functions and enums (encapsulation principle).
Also I think (and want) to rewrite the common.c. It has become very large and difficult to understand and support. I think that it should be divided into several files corresponding to the type of flash memory (STLINK_FLASH_TYPE_ *).
@Ant-ON that is a very good idea. There is another ticket that may be related to this: #237
@slyshykO and @Ant-ON: How do we proceed here? Is this issue still being actively worked on?
@Nightwalker-87 I have no spare time to work on this project now.
About this issue. I think we can close it now. But remember that ABI is important, and try not to break it by new commits.
@slyshykO Thx for the response.
But remember that ABI is important, and try not to break it by new commits.
As far as I understood from the context, it is not unified yet and/or has not been verified either. Thus the background of this issue is not resolved. However I understand that you, as the original issuer, are not able to contribute currently.
@Ant-ON Can you verify if this is implemented consistently throughout the whole code base?
@Ant-ON I was referring to the general status quo, not in reference to a certain release.
@Nightwalker-87 It is necessary to follow the PR and see that the fields are not add in the middle of the structure. Also we can write (add reference to this issue) about this in the development documentation.
Ok, we should do this then to finally address this issue. Are there any inconsistencies currently present that have not been taken note of yet?
@slyshykO Can you recheck the codebase for any leftovers by now? I'll add a respective note to the documentation soon. We should not close this issue before both of these tasks are done.
Hi @Nightwalker-87 . I am thinking about this issue.
At now, I propose to start the process of decreasing public headers.
We provide all headers that we have, and it accessible for all applications. The aim of decreasing should be only one public header - stlink.h. For example, see for binary of libusb.
After that, we will freeze the sizes of the public structs. By the way, we also should hide as many structs as we can.
So I propose to add milestone for this purpose and do steps toward the target. Cause it is impossible to do in one commit.
I did a quick look at the codebase and spot only one struct size change since 1.7.0.
It is struct stlink_chipid_params from chipid.h. It lost struct stlink_chipid_params * next; filed.
Ok, let's proceed as follows:
- Fix the latter and update the documentation on ABI and semantic versioning. This shall close this ticket.
- Open a separate ticket and introduce a working branch for the proposed structural change.
I'd not open a separate milestone for this but use the 1.8.0 milestone for this instead. If doing so please ensure to keep that working branch on track with develop and group some active developers to work on the topic together. What I'd like to avoid is that more complex changes are started and then cease to be actively worked on (for whatever reason) after a few weeks or months. We've seen that before and it leaves back incomplete implementations that might never complete. This is not good for the codebase and is an additional risk regarding regression. Please keep that in mind.
In my opinion, the other branch is a non-working option. Cause it is big refactoring. And it is impossible to have 2 synchronized branches with such a big diff.
As I said previously, we should walk to target by small, mergeable commits to the develop branch.
Well, to me it does not sound like a topic to push into v1.7.1 then, which should rather remain a bugfix and minor feature release. I think we should start this after 1.7.1 and then an in steps as proposed and together with the other big refactoring topic currently pending.
Seen from a more general perspective it would be favourable to resolve the oldest open bugfix topics in 1.7.x first to avoid dragging these along while the codebase is undergoing mayor rework. Fortunately their amount is constantly decreasing and I hope most of them can be solved within the 1.7.1 release.
@slyshykO Can you address https://github.com/stlink-org/stlink/issues/1075#issuecomment-899018590 then and take a closer look into the codebase as well? If you have any ideas on how to address this point in our documentation, you may post them here.
Without refactoring our codebase, we should mark in the readme that we break API every release, even minor ones. Also, we should link our binaries with static libstlink for all platforms. This prevents a runtime linkage with the wrong libstlink with another ABI.
@slyshykO Wait, what? As I read it, we were talking about ABI and not API in this issue. When looking at https://github.com/stlink-org/stlink/issues/1075#issuecomment-739563658 again such changes may have occurred several times and it would be impossible to root that back to any original state, nor preferable because a structure was likely never defined. How is the second mention related to this topic?
This issue is now closed due to inactivity.
Please also note that any version prior to v1.7.0 is unsupported according to our #Security Policy.
Should the problem persist, please retry with the latest version in the develop branch.
If this is the case, one should then open a new ticket.