-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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.
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 local labels in padlock.c #3452
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide the rationale for this change in the commit message and use "Fixes #3451". Also provide a ChangeLog entry.
done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changelog entry needs work.
The PR description says “needs backports” and “backported”, but no backports have been submitted. Are backports actually necessary here? Is there a risk that the change might not work in some environments (e.g. with older compilers or with toolchains other than GNU and Clang)? |
no backport please. |
Fixes Mbed-TLS#3451 Signed-off-by: okhowang(王沛文) <okhowang@tencent.com>
Changelog changed on demand. |
This fixes #3451 which is a failure to build in some cases, hence a bug, so I'm relabeling this from "enhancement" to "bug". (Also, the ChangeLog entry in this PR already says "bugfix", so let's be consistent.) As a general rule, bug fixes should be backported unless there's a specific reason not to. Do we have a reason not to backport this bug fix? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
I'm labeling "needs: backports" not to say that we need backports, but as a reminder that if we don't need them, we want to document why. |
symbol redefinition error is occurred in a condition which is unknown currently. |
My analysis on backporting: this is a platform incompatibility, so if it's already working for someone, there's a good chance that it'll keep working. This is a weak argument because it could stop working due to a compiler upgrade or, given the nature of the issue, due to a code change that affects how the function is inlined. On the other hand, there is a risk that some toolchains might not support the local label syntax, but again this is a weak argument since it seems that at least most Unixy assemblers do. As far as I understand, the problem arises if So I come out pretty close to the middle: there's some low value to backporting, and some low risk. The risk seems small, so I'm weakly in favor, but I'm fine with not backporting. That being said, backporting would take less time than this discussion is shaping up to take. |
Ok, so let's save time by not discussing this further and not backporting. At least now the reasons why we didn't backport are documented in case we (or someone else) wonder in the future why this wasn't backported. |
Hello! Is this something that could be considered for a backport to 2.16 LTS? We've been disabling padlock for a while in Godot due to this conflict, and there's no issue for us to keep patching it (we'll likely backport this patch to replace our own), but if this gets upstreamed in a future 2.16 release that's even better :) |
@akien-mga Mbed TLS 2.16 LTS has reached the end of its 3-year minimum support period and we are not extending it. As announced, last week's 2.16.12 release is the last in the 2.16 series. Please update to the new 2.28 LTS. |
@gilles-peskine-arm thank you! I had missed that message, it would be worth updating the 2.28.0 release notes to mention that this is the new LTS release. |
Description
fix #3451
Status
READY
Requires Backporting
NO
Todos
Steps to test or reproduce
Outline the steps to test or reproduce the PR here.