Skip to content
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

[PAL] Fix main() signature in regression tests for UBSan #2002

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

woju
Copy link
Member

@woju woju commented Sep 19, 2024

Description of the changes

Split from #1904

How to test this PR?

compile with ubsan on Ubuntu 24.04 and run


This change is Reviewable

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 17 of 17 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @woju)


pal/regression/AttestationReport.c line 12 at r1 (raw file):

int main(int argc, char** argv, char** envp) {
    /* We don't care about unused args to main, but UBSan complains otherwise
     * with call through pointer with incorrect function type"

Unmatched " at the end. ditto everywhere

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed 17 of 17 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow and @woju)


-- commits line 3 at r1:
I would also add the commit message body:

Newer Clang versions (v18 and newer) added more UBSan checks that
triggered UBSan on PAL regression tests. For more info, see previous
commit "[common] Update UBSan to be compatible with Clang 18".

pal/regression/AttestationReport.c line 12 at r1 (raw file):

Previously, mkow (Michał Kowalczyk) wrote…

Unmatched " at the end. ditto everywhere

Yes, should be with "call through pointer ..."

Also, I personally don't like when for 1- or 2-line comments, the ending bracket is on a separate line. I would finish in the same (second) line: ...function type" */

@woju woju force-pushed the woju/fix-pal-regression-ubsan branch from 8cdb22f to 097343f Compare September 20, 2024 11:30
Copy link
Member Author

@woju woju left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 17 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv and @mkow)


-- commits line 3 at r1:

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

I would also add the commit message body:

Newer Clang versions (v18 and newer) added more UBSan checks that
triggered UBSan on PAL regression tests. For more info, see previous
commit "[common] Update UBSan to be compatible with Clang 18".

Done.


pal/regression/AttestationReport.c line 12 at r1 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Yes, should be with "call through pointer ..."

Also, I personally don't like when for 1- or 2-line comments, the ending bracket is on a separate line. I would finish in the same (second) line: ...function type" */

Done.

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed 16 of 16 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @mkow and @woju)


pal/regression/ipv6_parser.c line 1 at r2 (raw file):

/* SPDX-License-Identifier: LGPL-3.0-or-later */ /* Copyright (C) 2022 Intel Corporation

Unintended bug?

Newer Clang versions (v18 and newer) added more UBSan checks that
triggered UBSan on PAL regression tests. For more info, see commit
78776a5 ("[common] Update UBSan to be compatible with Clang 18").

Signed-off-by: Wojtek Porczyk <woju@invisiblethingslab.com>
@woju woju force-pushed the woju/fix-pal-regression-ubsan branch from 097343f to 8825c76 Compare September 20, 2024 11:40
Copy link
Member Author

@woju woju left a comment

Choose a reason for hiding this comment

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

Reviewable status: 16 of 17 files reviewed, 2 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (1 more required, approved so far: ITL) (waiting on @dimakuv and @mkow)


pal/regression/ipv6_parser.c line 1 at r2 (raw file):

Previously, dimakuv (Dmitrii Kuvaiskii) wrote…

Unintended bug?

Fixed. Yes, too much joining lines

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required) (waiting on @mkow)

Copy link
Member

@mkow mkow left a comment

Choose a reason for hiding this comment

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

Reviewed 15 of 16 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@kailun-qin kailun-qin left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 17 files at r1, 15 of 16 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@woju
Copy link
Member Author

woju commented Sep 20, 2024

Jenkins, test linux-sgx-vm-gcc-release please

1 similar comment
@woju
Copy link
Member Author

woju commented Sep 20, 2024

Jenkins, test linux-sgx-vm-gcc-release please

Copy link
Contributor

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@woju
Copy link
Member Author

woju commented Sep 20, 2024

Jenkins, test linux-sgx-vm-gcc-release please

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

Successfully merging this pull request may close these issues.

4 participants