Skip to content

Fix fatal parameter error when deleting/terminating Thread object #5587

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

Merged
merged 2 commits into from
Dec 20, 2017

Conversation

marogi
Copy link
Contributor

@marogi marogi commented Nov 25, 2017

Description

Call to osThreadTerminate is guarded by local_id != 0 check, to avoid parameter error fault when deleting or terminating Thread object that was not started.

Status

READY

Migrations

NO

Todos

  • Tests

Steps to test or reproduce

On any target (tested with FRDM-K64F):

#include "mbed.h"

RawSerial output(USBTX, USBRX);

int main(int argc, char **argv)
{
    output.baud(115200);

    output.printf("\r\nStarting mbed\r\n");

    {
        rtos::Thread dummyToTriggerError(osPriorityNormal, 0x500);
    }

    while(1);
}

output:

Starting mbed
Thread 0x0 error -4: Parameter error

Call to osThreadTerminate is guarded by local_id check, to avoid parameter error fault when deleting or terminating Thread object that was not started.
@0xc0170 0xc0170 requested review from bulislaw and c1728p9 November 27, 2017 08:17
rtos/Thread.cpp Outdated
ret = osThreadTerminate(local_id);
// if local_id == 0 Thread was not started in first place
// and does not have to be terminated
if(local_id != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if () - please leave as space there

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 27, 2017

@marogi Can you please sign https://os.mbed.com/contributor_agreement/?

@marogi
Copy link
Contributor Author

marogi commented Nov 27, 2017

Sure - whitespace fixed.

@ Contributor agreement: I have it signed on my mbed accout (devmarogi, linked to this GitHub account). Any other way I need to sign it?

Copy link
Member

@bulislaw bulislaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 27, 2017

/morph build

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 27, 2017

@marogi 👍 for the fix

@mbed-ci
Copy link

mbed-ci commented Nov 27, 2017

Build : SUCCESS

Build number : 598
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5587/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci
Copy link

mbed-ci commented Nov 27, 2017

@mbed-ci
Copy link

mbed-ci commented Nov 27, 2017

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 27, 2017

Seems like device problem ,will restart once fixed, going to investigate

@theotherjimmy
Copy link
Contributor

/morph uvisor-test

@kegilbert
Copy link
Contributor

@0xc0170 Updated some of the CI hardware to hopefully resolve the F429ZI flash errors. I'll rekick this off ASAP.

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 27, 2017

@marogi - can you or slashdevteam sign to travis-ci, it does not recognize you, and labels this PR as abuse.

@marogi
Copy link
Contributor Author

marogi commented Nov 27, 2017

@0xc0170 : This fix is really not abuse :)

I saw the abuse status on travis-ci, but once I logged in (via GitHub) it was gone. Is there some other way I can authenticate me/slashdev?

@kegilbert
Copy link
Contributor

/morph test

@mbed-ci
Copy link

mbed-ci commented Nov 28, 2017

@marogi
Copy link
Contributor Author

marogi commented Nov 28, 2017

I contacted travis-ci and apparently pull requests from GitHub accounts that never logged into travis-ci will get blocked. They also recommended closing and reopening pull request.

I connected my account to travis-ci, so @0xc0170 could you try to close & reopen this PR?

@0xc0170 0xc0170 closed this Nov 28, 2017
@0xc0170 0xc0170 removed the needs: CI label Nov 28, 2017
@0xc0170 0xc0170 reopened this Nov 28, 2017
@0xc0170
Copy link
Contributor

0xc0170 commented Nov 28, 2017

Still flagged as abuse 😭 New PR might resolve it? Thanks @marogi for the logging in.

@marogi
Copy link
Contributor Author

marogi commented Nov 28, 2017

@0xc0170 no problem.

I reported this to travis-ci and will work with them on solution.

@mbed-ci
Copy link

mbed-ci commented Nov 28, 2017

Build : SUCCESS

Build number : 609
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5587/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci
Copy link

mbed-ci commented Nov 28, 2017

@mbed-ci
Copy link

mbed-ci commented Nov 28, 2017

@mbed-ci
Copy link

mbed-ci commented Nov 28, 2017

@0xc0170
Copy link
Contributor

0xc0170 commented Nov 30, 2017

@marogi Have you received any response from travis-ci ? I can help to cherry-pick this change and send a new PR for now, let me know please

@marogi
Copy link
Contributor Author

marogi commented Nov 30, 2017

I'm waiting on one more reply from travis-ci. If it doesn't come tomorrow (or does not solve the issue) I will close this PR and create new one from a different fork (my own, not under slashdevteam).

@marogi
Copy link
Contributor Author

marogi commented Dec 1, 2017

Ok, can we make last one try? This time all settings should make travis-ci happy.

@0xc0170 0xc0170 closed this Dec 8, 2017
@0xc0170 0xc0170 reopened this Dec 8, 2017
@0xc0170
Copy link
Contributor

0xc0170 commented Dec 8, 2017

Travis works !

@mbed-ci
Copy link

mbed-ci commented Dec 8, 2017

Build : SUCCESS

Build number : 672
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/5587/

Triggering tests

/morph test
/morph uvisor-test
/morph export-build

@mbed-ci
Copy link

mbed-ci commented Dec 8, 2017

1 similar comment
@mbed-ci
Copy link

mbed-ci commented Dec 8, 2017

@kegilbert
Copy link
Contributor

/morph test

@mbed-ci
Copy link

mbed-ci commented Dec 8, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants