intrinsics macro: fix non-weak aeabi generation #552
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When the
weak-intrinsics
feature landed in #526, my reading of the intent as described in the issue is that theweak-intrinsics
feature is to be disabled by default, and that when disabled it should essentially be a no-op / no outwardly observable change (though I could be mistaken).Taking a look at this particular line, a hypothesis is that this was a typo in terms of parenthesis placement, as we can see that the output of
cargo expand float --target arm-unknown-linux-musleabi
changes before / after that change landed:diff between before / after
We encountered this when attempting to update a project from rust 1.70 -> 1.71, where 1.71 pulled in
compiler-builtins
0.1.92 / the aforementioned change. I've excised some of the output so this is not the full contents, but we essentially see this error at link time:And that error is repeated for
__aeabi_fadd
,__aeabi_ui2f
,__aeabi_ul2f
,__aeabi_l2f
.We can see that when applying the fix in this PR that this undoes the delta from above:
diff between current / fix
Also of note is that building a 1.71 rust compiler whose only difference was to pull in this change ended up resolving the multiple definitions error we encountered above.
I'm curious on if there's any prior art in potentially adding automated testing around this fix within
testcrate
, not sure if there are any insightful ideas around this?If it is preferred I make an issue for this or any other modifications to the contribution process, please just let me know, thanks in advance!