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

build: Use -fcf-protection for GCC >= 8 or Clang >= 7 #944

Closed
wants to merge 1 commit into from

Conversation

Krechals
Copy link
Contributor

@Krechals Krechals commented Jun 13, 2023

Prerequisite checklist

  • Read the contribution guidelines regarding submitting new changes to the project;
  • Tested your changes against relevant architectures and platforms;
  • Ran the checkpatch.uk on your commit series before opening this PR;
  • Updated relevant documentation.

Base target

  • Architecture(s): [e.g. x86_64 or N/A]
  • Platform(s): [e.g. kvm, xen or N/A]
  • Application(s): [e.g. app-python3 or N/A]

Additional configuration

Description of changes

The build fails on every app when gcc-8.0+ isn't used.

-fcf-protection was introduced in gcc-8.0 and in clang-7.0.

The build still fails for clang-7.0, since __asm__ goto from plat/kvm/x86/traps.c was introduced in clang-9.0.

GitHub-Depends-On: #983

@Krechals Krechals requested a review from a team as a code owner June 13, 2023 14:01
@unikraft-bot unikraft-bot added the area/makefile Part of the Unikraft Makefile build system label Jun 13, 2023
@razvand razvand requested review from StefanJum and mariasfiraiala and removed request for a team and adinasm June 15, 2023 15:45
@razvand razvand assigned razvand and unassigned nderjung Jun 15, 2023
@razvand razvand added kind/quick-fix Issue is a quick fix topic/build Topics to do with the build system labels Jun 15, 2023
@razvand razvand added this to the v0.14.0 (Prometheus) milestone Jun 15, 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.

This looks fine @Krechals, thank you, see the one comment.

About the build still failing with clang 7, should we add a global check for the clang version and fail if it's <= 9 (it could be only for x86_64 or whatever setup fails to build)?
Something like this.

--- LATER EDIT: This will be done by #949

I would also split the PR into 2 commits, one that adds the -fcf-protection flag and one that adds the error_if_clang_version_lt rule to the build system.

support/build/Makefile.rules Outdated Show resolved Hide resolved
@razvand
Copy link
Contributor

razvand commented Jul 17, 2023

@Krechals, could you please take a look in this PR to have it ready for upstream? See @StefanJum's comments. @mariasfiraiala , please also take a look.

Copy link
Contributor

@mariasfiraiala mariasfiraiala left a comment

Choose a reason for hiding this comment

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

This works fine, thanks! @Krechals can you please address @StefanJum's comments in order to have this merged? I think they make sense.

@razvand
Copy link
Contributor

razvand commented Jul 24, 2023

@Krechals , could you please rebase this PR on top of #983 ? Simply add the -fcf-protection specific checks commit and use the Clang-specific Makefile updates from #983 .

@razvand
Copy link
Contributor

razvand commented Jul 27, 2023

@Krechals , could you rebase this, now that PR #983 is in?

Copy link
Contributor

@mariasfiraiala mariasfiraiala 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, thanks!

Reviewed-by: Maria Sfiraiala maria.sfiraiala@gmail.com

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.

This commit adds a minimum compiler version check, which is 9.0 for clang. So we only need the check for gcc.

Something like

ifeq ($(call have_gcc),y)
COMPFLAGS-$(call gcc_version_ge,8,0)	+= -fcf-protection=none
else
COMPFLAGS-y += -fcf-protection=none
endif

or

COMPFLAGS-$(call gcc_version_ge,8,0)	+= -fcf-protection=none
COMPFLAGS-$(call have_clang) += -fcf-protection=none

could be better?

@StefanJum
Copy link
Member

All good now @Krechals, just please remove the id (101668) from the commit authored-by and sign-off tags, we can merge after that.

Signed-off-by: Andrei Tudor TOPALĂ <topala.andrei@gmail.com>
@unikraft-bot
Copy link
Member

Checkpatch passed

Beep boop! I ran Unikraft's checkpatch.pl support script on your pull request and it all looks good!

SHA commit checkpatch
2906df3 build: Use -fcf-protection for GCC >= 8 or Clang >= 7

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, thanks.

Reviewed-by: Stefan Jumarea stefanjumarea02@gmail.com

Copy link
Contributor

@razvand razvand left a comment

Choose a reason for hiding this comment

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

Approved-by: Razvan Deaconescu razvand@unikraft.io

@unikraft-bot unikraft-bot added the ci/merged Merged by CI label Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/makefile Part of the Unikraft Makefile build system ci/merged Merged by CI kind/quick-fix Issue is a quick fix size/small Small PR, quick to review topic/build Topics to do with the build system
Projects
Status: Done
Status: Done!
Development

Successfully merging this pull request may close these issues.

None yet

6 participants