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

Makefile.uk: Don't add unsupported warnings when built with clang #10

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

Conversation

iAmGroute
Copy link

Part of the hackathon challenges, Fixes: #1

I couldn't reproduce the original issue with -Wno-unused-but-set-variable -Wno-maybe-uninitialized, but verified with -Wno-builtin-declaration-mismatch.

Authored by @Se7enBit, I'm just doing the commit/PR :)

@razvand
Copy link
Contributor

razvand commented Apr 16, 2023

Hackathon points: 20

@razvand razvand requested a review from StefanJum May 4, 2023 14:31
@razvand razvand added the enhancement New feature or request label May 4, 2023
@razvand razvand added this to the v0.13.0 (Atlas) milestone May 4, 2023
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Thank you @iAmGroute, besides the comments. please also add a short description to the commit message.

Makefile.uk Outdated Show resolved Hide resolved
Makefile.uk Outdated Show resolved Hide resolved
Makefile.uk Outdated Show resolved Hide resolved
@razvand
Copy link
Contributor

razvand commented May 10, 2023

@iAmGroute, could you please take care of the comments above?

@iAmGroute
Copy link
Author

iAmGroute commented May 10, 2023

Hi @StefanJum, sorry for the delay.
I've made the requested changes and amended the commit.

For reference, when built:

  • with CC=gcc , it runs: gcc [..] -Wno-unused-parameter -Wno-unused-value -Wno-parentheses -Wno-error=sign-compare -Wno-builtin-macro-redefined [..] -Wno-unused-but-set-variable -Wno-maybe-uninitialized -Wno-builtin-declaration-mismatch -Wno-unused-parameter [..]
  • with CC=clang, it runs: clang [..] -Wno-unused-parameter -Wno-unused-value -Wno-parentheses -Wno-error=sign-compare -Wno-builtin-macro-redefined [..] -Wno-unused-parameter [..]

I've tested the above with GCC and Clang, it's as expected:

$ gcc   -Wno-unused-but-set-variable -Wno-maybe-uninitialized -Wno-builtin-declaration-mismatch test.c
$ #no warnings
$
$ clang -Wno-unused-but-set-variable -Wno-maybe-uninitialized -Wno-builtin-declaration-mismatch test.c
warning: unknown warning option '-Wno-unused-but-set-variable'; did you mean '-Wno-unused-const-variable'? [-Wunknown-warning-option]
warning: unknown warning option '-Wno-maybe-uninitialized'; did you mean '-Wno-uninitialized'? [-Wunknown-warning-option]
warning: unknown warning option '-Wno-builtin-declaration-mismatch'; did you mean '-Wno-missing-declarations'? [-Wunknown-warning-option]
3 warnings generated.
$
$ clang test.c
$ #no warnings
$

Versions:

$ gcc --version
gcc (Ubuntu 11.3.0-1ubuntu1~22.04) 11.3.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ clang --version
Ubuntu clang version 12.0.1-19ubuntu3
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

@razvand razvand requested a review from StefanJum May 12, 2023 12:19
Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

Hi @iAmGroute, please wrap your commit message at ~72 characters.
Also it looks like there are some conflicts, so please rebase it. We can merge it after that.

Clang (12.0.0) does not recognize the warnings:
  no-unused-but-set-variable
  no-maybe-uninitialized
  no-builtin-declaration-mismatch
It would print "unknown warning option" warning messages.
These should be enabled only with GCC (tested with 11.3.0).

GitHub-Fixes: unikraft#1

Co-authored-by: Nikos Oikonomou <[email protected]>
Signed-off-by: Nikos Oikonomou <[email protected]>
Signed-off-by: Groute <[email protected]>
@iAmGroute
Copy link
Author

Should be OK now.

Copy link
Member

@StefanJum StefanJum left a comment

Choose a reason for hiding this comment

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

All good now, thanks.
Reviewed-by: Stefan Jumarea [email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Warning options need to be updated when compiling with clang
5 participants