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

[AIX] Turn on #pragma mc_func check by default #101336

Merged
merged 2 commits into from
Aug 1, 2024

Conversation

qiongsiwu
Copy link
Contributor

#99888 added a check (and corresponding options) to flag uses of #pragma mc_func on AIX.

This PR turns on the check by default.

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:PowerPC clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jul 31, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jul 31, 2024

@llvm/pr-subscribers-clang-driver
@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-powerpc

Author: Qiongsi Wu (qiongsiwu)

Changes

#99888 added a check (and corresponding options) to flag uses of #pragma mc_func on AIX.

This PR turns on the check by default.


Full diff: https://github.com/llvm/llvm-project/pull/101336.diff

4 Files Affected:

  • (modified) clang/include/clang/Driver/Options.td (+1-1)
  • (modified) clang/include/clang/Lex/PreprocessorOptions.h (+2-2)
  • (modified) clang/lib/Driver/ToolChains/AIX.cpp (+1-1)
  • (modified) clang/test/Preprocessor/pragma_mc_func.c (+2-2)
diff --git a/clang/include/clang/Driver/Options.td b/clang/include/clang/Driver/Options.td
index c8c56dbb51b28..7f6b8ba4d7eeb 100644
--- a/clang/include/clang/Driver/Options.td
+++ b/clang/include/clang/Driver/Options.td
@@ -8097,7 +8097,7 @@ def source_date_epoch : Separate<["-"], "source-date-epoch">,
 } // let Visibility = [CC1Option]
 
 defm err_pragma_mc_func_aix : BoolFOption<"err-pragma-mc-func-aix",
-  PreprocessorOpts<"ErrorOnPragmaMcfuncOnAIX">, DefaultFalse,
+  PreprocessorOpts<"ErrorOnPragmaMcfuncOnAIX">, DefaultTrue,
   PosFlag<SetTrue, [], [ClangOption, CC1Option],
           "Treat uses of #pragma mc_func as errors">,
   NegFlag<SetFalse,[], [ClangOption, CC1Option],
diff --git a/clang/include/clang/Lex/PreprocessorOptions.h b/clang/include/clang/Lex/PreprocessorOptions.h
index 3f7dd9db18ba7..f48b7ecb90e1e 100644
--- a/clang/include/clang/Lex/PreprocessorOptions.h
+++ b/clang/include/clang/Lex/PreprocessorOptions.h
@@ -213,7 +213,7 @@ class PreprocessorOptions {
 
   /// If set, the preprocessor reports an error when processing #pragma mc_func
   /// on AIX.
-  bool ErrorOnPragmaMcfuncOnAIX = false;
+  bool ErrorOnPragmaMcfuncOnAIX = true;
 
 public:
   PreprocessorOptions() : PrecompiledPreambleBytes(0, false) {}
@@ -252,7 +252,7 @@ class PreprocessorOptions {
     PrecompiledPreambleBytes.first = 0;
     PrecompiledPreambleBytes.second = false;
     RetainExcludedConditionalBlocks = false;
-    ErrorOnPragmaMcfuncOnAIX = false;
+    ErrorOnPragmaMcfuncOnAIX = true;
   }
 };
 
diff --git a/clang/lib/Driver/ToolChains/AIX.cpp b/clang/lib/Driver/ToolChains/AIX.cpp
index b2885b7776d13..f0e7acac688fc 100644
--- a/clang/lib/Driver/ToolChains/AIX.cpp
+++ b/clang/lib/Driver/ToolChains/AIX.cpp
@@ -562,7 +562,7 @@ void AIX::addClangTargetOptions(
     CC1Args.push_back("-fno-sized-deallocation");
 
   if (Args.hasFlag(options::OPT_ferr_pragma_mc_func_aix,
-                   options::OPT_fno_err_pragma_mc_func_aix, false))
+                   options::OPT_fno_err_pragma_mc_func_aix, true))
     CC1Args.push_back("-ferr-pragma-mc-func-aix");
   else
     CC1Args.push_back("-fno-err-pragma-mc-func-aix");
diff --git a/clang/test/Preprocessor/pragma_mc_func.c b/clang/test/Preprocessor/pragma_mc_func.c
index f0d3e49e5dddc..8d172f8490e25 100644
--- a/clang/test/Preprocessor/pragma_mc_func.c
+++ b/clang/test/Preprocessor/pragma_mc_func.c
@@ -1,3 +1,4 @@
+// RUN: not %clang --target=powerpc64-ibm-aix -fsyntax-only %s 2>&1 | FileCheck %s
 // RUN: not %clang --target=powerpc64-ibm-aix -ferr-pragma-mc-func-aix -fsyntax-only \
 // RUN:   %s 2>&1 | FileCheck %s
 #pragma mc_func asm_barrier {"60000000"}
@@ -8,11 +9,10 @@
 // RUN: %clang --target=powerpc64-ibm-aix -fno-err-pragma-mc-func-aix -fsyntax-only %s
 // RUN: %clang --target=powerpc64-ibm-aix -ferr-pragma-mc-func-aix -fsyntax-only \
 // RUN:    -fno-err-pragma-mc-func-aix %s
-// RUN: %clang --target=powerpc64-ibm-aix -fsyntax-only %s
 // RUN: %clang --target=powerpc64-ibm-aix -Werror=unknown-pragmas \
 // RUN:   -fno-err-pragma-mc-func-aix -fsyntax-only %s
 
-// Cases where we have errors or warnings.
+// Cases on a non-AIX target.
 // RUN: not %clang --target=powerpc64le-unknown-linux-gnu \
 // RUN:   -Werror=unknown-pragmas -fno-err-pragma-mc-func-aix -fsyntax-only %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=UNUSED %s

@@ -562,7 +562,7 @@ void AIX::addClangTargetOptions(
CC1Args.push_back("-fno-sized-deallocation");

if (Args.hasFlag(options::OPT_ferr_pragma_mc_func_aix,
options::OPT_fno_err_pragma_mc_func_aix, false))
options::OPT_fno_err_pragma_mc_func_aix, true))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since the preprocessor has a default value, can we avoid the cc1 option if the value here matches the default value inside the preprocessor, like:

if (Args.hasArg(options::OPT_fno_err_pragma_mc_func_aix))
   CC1Args.push_back("-fno-err-pragma-mc-func-aix");

(no need to pass -ferr-pragma-mc-func-aix as it is the default inside preprocessor.)

Copy link
Contributor Author

@qiongsiwu qiongsiwu Jul 31, 2024

Choose a reason for hiding this comment

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

Good point! Code is revised.

I tried the suggested change

if (Args.hasArg(options::OPT_fno_err_pragma_mc_func_aix))
   CC1Args.push_back("-fno-err-pragma-mc-func-aix");

But it does not handle situations like -fno-err-pragma-mc-func-aix -ferr-pragma-mc-func-aix correctly (or a single -ferr-pragma-mc-func-aix). I think we still need to process both flags in the driver, but we only need to pass -fno-err-pragma-mc-func-aix to CC1. Did I miss something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your code is the correct one. Thanks for pointing this out.

Copy link
Collaborator

@chenzheng1030 chenzheng1030 left a comment

Choose a reason for hiding this comment

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

LGTM. Please wait for @AaronBallman 's approval. Thanks

Copy link
Collaborator

@AaronBallman AaronBallman left a comment

Choose a reason for hiding this comment

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

LGTM! Please make sure 1) this gets backported to the 19.x branch (https://www.llvm.org/docs/GitHub.html#backporting-fixes-to-the-release-branches), 2) the 19.x release branch gets a release note for the pragma and command line options.

@qiongsiwu qiongsiwu merged commit b933517 into llvm:main Aug 1, 2024
7 checks passed
@qiongsiwu qiongsiwu added this to the LLVM 19.X Release milestone Aug 1, 2024
@qiongsiwu
Copy link
Contributor Author

/cherry-pick b933517

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 1, 2024
llvm#99888 added a check (and
corresponding options) to flag uses of `#pragma mc_func` on AIX.

This PR turns on the check by default.

(cherry picked from commit b933517)
@llvmbot
Copy link
Collaborator

llvmbot commented Aug 1, 2024

/pull-request #101505

tru pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 2, 2024
llvm#99888 added a check (and
corresponding options) to flag uses of `#pragma mc_func` on AIX.

This PR turns on the check by default.

(cherry picked from commit b933517)
tru pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 2, 2024
llvm#99888 added a check (and
corresponding options) to flag uses of `#pragma mc_func` on AIX.

This PR turns on the check by default.

(cherry picked from commit b933517)
@qiongsiwu
Copy link
Contributor Author

@AaronBallman sorry to bug you. This patch was not tested sufficiently on my end and it is causing build failures on AIX buildbots due to the aforementioned system headers including #pragma mc_func. See https://lab.llvm.org/buildbot/#/builders/64/builds/582/steps/5/logs/stdio for example. The compiler-rt cmake files need some updates to add the no-err flag. Additionally, there are tests that needs to be updated (we need to augment the tests with -fno-err-pragma-mc-func-aix e.g. https://lab.llvm.org/buildbot/#/builders/64/builds/550). Do you think it is reasonable to revert this PR for now (and from llvm19)? We can reland the PR once these necessary changes are made in one batch together?

Thanks for your input!

qiongsiwu pushed a commit that referenced this pull request Aug 2, 2024
@qiongsiwu
Copy link
Contributor Author

qiongsiwu commented Aug 2, 2024

Pushed dd7a4c3 to revert this PR for the moment since it is causing too many build and test failures (upstream and downstream). The failures are all caused by AIX headers including #prgama mc_func. Will revert this PR from llvm19 release branch as well.

/cherry-pick dd7a4c3

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 2, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Aug 2, 2024

/pull-request #101737

@MaskRay
Copy link
Member

MaskRay commented Aug 2, 2024

(Tips: Personally when I have some patches intended for the release branch, I'll build and test llvm-project in the release branch as well to decrease the risk of potential immediate reverts/churn. An revert should state reasons.)

llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 2, 2024
tru pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 4, 2024
@AaronBallman
Copy link
Collaborator

@AaronBallman sorry to bug you. This patch was not tested sufficiently on my end and it is causing build failures on AIX buildbots due to the aforementioned system headers including #pragma mc_func. See https://lab.llvm.org/buildbot/#/builders/64/builds/582/steps/5/logs/stdio for example. The compiler-rt cmake files need some updates to add the no-err flag. Additionally, there are tests that needs to be updated (we need to augment the tests with -fno-err-pragma-mc-func-aix e.g. https://lab.llvm.org/buildbot/#/builders/64/builds/550). Do you think it is reasonable to revert this PR for now (and from llvm19)? We can reland the PR once these necessary changes are made in one batch together?

Thanks for your input!

I think it would make sense to revert this and the original patch (so there's no #pragma mc_func support in Clang 19.x or main at all) so we can take a fresh run at a new PR to see all of the impacts and consider the design. My concern with reverting just this bit and not the original PR is that we'll leave the original functionality as-is permanently despite the design concerns with it being off-by-default functionality. WDYT?

@cor3ntin
Copy link
Contributor

cor3ntin commented Aug 5, 2024

Should we consider a feature like -Wunknown-pragmas=<pragma-name> rather than doing something specific for that one pragma?
Have you consider whether this use case generalizes at all?

banach-space pushed a commit to banach-space/llvm-project that referenced this pull request Aug 7, 2024
llvm#99888 added a check (and
corresponding options) to flag uses of `#pragma mc_func` on AIX.

This PR turns on the check by default.
banach-space pushed a commit to banach-space/llvm-project that referenced this pull request Aug 7, 2024
@qiongsiwu
Copy link
Contributor Author

Sorry about the delay! I was on break. Thanks for the comments @AaronBallman and @cor3ntin !

I think it would make sense to revert this and the original patch (so there's no #pragma mc_func support in Clang 19.x or main at all) so we can take a fresh run at a new PR to see all of the impacts and consider the design.

I have relayed the comments internally and we are discussing it. I will let you know as soon as possible.

Should we consider a feature like -Wunknown-pragmas= rather than doing something specific for that one pragma?
Have you consider whether this use case generalizes at all?

No we were not aware of other use cases. But -Wunknown-pragmas=<pragma-name> sounds like a good idea! Do you think this can be useful in other cases? How do we envision this warning/error to work with the existing unknown-pragmas warning if they are both present? I will draft a discourse RFC if this is sufficiently general/useful.

@AaronBallman
Copy link
Collaborator

AaronBallman commented Aug 9, 2024

I have relayed the comments internally and we are discussing it. I will let you know as soon as possible.

Thank you! However, we're getting pretty late in the release cycle and so I'd like to see this backed out of 19.x ASAP (and given the direction the discussion is going, I think it should be backed out of main as well).

No we were not aware of other use cases. But -Wunknown-pragmas= sounds like a good idea! Do you think this can be useful in other cases? How do we envision this warning/error to work with the existing unknown-pragmas warning if they are both present? I will draft a discourse RFC if this is sufficiently general/useful.

I think this would be useful for both pragmas and attributes (with -Wunknown-attribute= as well), but it might be somewhat more involved to implement because IIRC we have to treat those warnings as command line options directly (in Options.td) in order to get the correct behavior.

I would imagine that we'd want last-flag-wins behavior for cases like -Wunknown-pragmas=foo -Wno-unknown-pragmas (silences all pragma warnings) or -Wno-unknown-pragmas -Wunknown-pragmas=foo (silences all pragma warnings except for #pragma foo). Similar for attributes. We probably also want to support a delimited list of pragmas/attributes to ignore, like -Wno-unknown-attributes=foo,bar,baz. I don't think we need to care about the syntax used for the attribute (__attribute__ vs [[]] vs __declspec), just the attribute name. But it does mean we have to think about cases where the same attribute name is under different prefixes, like clang::builtin_alias and clang_builtin_alias or clang::no_stack_protector and gnu::no_stack_protector.

@AaronBallman
Copy link
Collaborator

Thank you! However, we're getting pretty late in the release cycle and so I'd like to see this backed out of 19.x ASAP (and given the direction the discussion is going, I think it should be backed out of main as well).

@qiongsiwu will you be able to get these changes reverted today? If not, I'll try to revert them this afternoon. CC @tstellar @tru for release manager awareness.

@qiongsiwu
Copy link
Contributor Author

Thank you! However, we're getting pretty late in the release cycle and so I'd like to see this backed out of 19.x ASAP (and given the direction the discussion is going, I think it should be backed out of main as well).

@qiongsiwu will you be able to get these changes reverted today? If not, I'll try to revert them this afternoon. CC @tstellar @tru for release manager awareness.

Yes I will get to this later today. We have discussed and it is OK at the moment to pull out this check. I will open a PR soon (to revert all the commits relevant to the check).

@qiongsiwu
Copy link
Contributor Author

#102919 is created to revert this feature now. I will review our discussion and consider an improved implementation. Thanks again for the input and discussion!

qiongsiwu added a commit that referenced this pull request Aug 12, 2024
#99888 added a specific
diagnostic for `#pragma mc_func` on AIX. There are some disagreements
on:

1. If the check should be on by default. Leaving the check off by
default is dangerous, since it is difficult to be aware of such a check.
Turning it on by default at the moment causes build failures on AIX. See
#101336 for more details.
2. If the check can be made more general. See
#101336 (comment).

This PR reverts this check from `main` so we can flush out these
disagreements.
llvmbot pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 12, 2024
llvm#99888 added a specific
diagnostic for `#pragma mc_func` on AIX. There are some disagreements
on:

1. If the check should be on by default. Leaving the check off by
default is dangerous, since it is difficult to be aware of such a check.
Turning it on by default at the moment causes build failures on AIX. See
llvm#101336 for more details.
2. If the check can be made more general. See
llvm#101336 (comment).

This PR reverts this check from `main` so we can flush out these
disagreements.

(cherry picked from commit 123b6fc)
tru pushed a commit to llvmbot/llvm-project that referenced this pull request Aug 13, 2024
llvm#99888 added a specific
diagnostic for `#pragma mc_func` on AIX. There are some disagreements
on:

1. If the check should be on by default. Leaving the check off by
default is dangerous, since it is difficult to be aware of such a check.
Turning it on by default at the moment causes build failures on AIX. See
llvm#101336 for more details.
2. If the check can be made more general. See
llvm#101336 (comment).

This PR reverts this check from `main` so we can flush out these
disagreements.

(cherry picked from commit 123b6fc)
bwendling pushed a commit to bwendling/llvm-project that referenced this pull request Aug 15, 2024
llvm#99888 added a specific
diagnostic for `#pragma mc_func` on AIX. There are some disagreements
on:

1. If the check should be on by default. Leaving the check off by
default is dangerous, since it is difficult to be aware of such a check.
Turning it on by default at the moment causes build failures on AIX. See
llvm#101336 for more details.
2. If the check can be made more general. See
llvm#101336 (comment).

This PR reverts this check from `main` so we can flush out these
disagreements.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:PowerPC clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
Development

Successfully merging this pull request may close these issues.

6 participants