- 
                Notifications
    
You must be signed in to change notification settings  - Fork 15.1k
 
[Clang][BPF] Add __BPF_FEATURE_GOTOX #165456
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
Conversation
Add a macro __BPF_FEATURE_GOTOX for bpf target for cpu v4. So the developer can easily detect whether insn gotox is supported or not.
| 
          
 @llvm/pr-subscribers-clang Author: None (yonghong-song) ChangesAdd a macro __BPF_FEATURE_GOTOX for bpf target for cpu v4. So the developer can easily detect whether insn gotox is supported or not. Full diff: https://github.com/llvm/llvm-project/pull/165456.diff 2 Files Affected: 
 diff --git a/clang/lib/Basic/Targets/BPF.cpp b/clang/lib/Basic/Targets/BPF.cpp
index 0411bcca51789..8de1083d758c7 100644
--- a/clang/lib/Basic/Targets/BPF.cpp
+++ b/clang/lib/Basic/Targets/BPF.cpp
@@ -75,6 +75,7 @@ void BPFTargetInfo::getTargetDefines(const LangOptions &Opts,
     Builder.defineMacro("__BPF_FEATURE_GOTOL");
     Builder.defineMacro("__BPF_FEATURE_ST");
     Builder.defineMacro("__BPF_FEATURE_LOAD_ACQ_STORE_REL");
+    Builder.defineMacro("__BPF_FEATURE_GOTOX");
   }
 }
 
diff --git a/clang/test/Preprocessor/bpf-predefined-macros.c b/clang/test/Preprocessor/bpf-predefined-macros.c
index cd8a2ec031925..a9ae8c58c3ba7 100644
--- a/clang/test/Preprocessor/bpf-predefined-macros.c
+++ b/clang/test/Preprocessor/bpf-predefined-macros.c
@@ -70,6 +70,9 @@ int u;
 #ifdef __BPF_FEATURE_LOAD_ACQ_STORE_REL
 int v;
 #endif
+#ifdef __BPF_FEATURE_GOTOX
+int w;
+#endif
 
 // CHECK: int b;
 // CHECK: int c;
@@ -110,6 +113,7 @@ int v;
 // CPU_V4: int u;
 
 // CPU_V4: int v;
+// CPU_V4: int w;
 
 // CPU_GENERIC: int g;
 
 | 
    
| 
           cc @aspsk  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we come to a point where we need to decide if gotox is a part of cpuv4. If it is, we should keep it disabled by default, to avoid backwards compatibility issues.
@4ast , @yonghong-song
| 
           I think it should be okay. Based on practice of selftest, feature testing sounds a good way to guard specific features. And in the long run, e.g., after llvm24 released, we do not even care to use feature guard as users can upgrade to llvm22/23/24.  | 
    
          
 Yes, but if someone upgrades to new llvm he would have to either adjust build scripts to disable   | 
    
| 
           I think it should be okay. I discussed this earlier with @4ast, we do not to increase version number frequently as it is not sustainable. So we have feature flags and llvm internal flags to guard this. Just for this specific case, switch cases need to more than 12 to trigger jump table and computed goto won't work without this feature. So potential hits on people is low. @4ast What do you think?  | 
    
          
 it's fine for gotox to be in cpuv4. It will be generated only for large-ish switches where backward compat is unlikely to be an issue, and for explicit computed goto which would have failed to compile.  | 
    
| 
           LLVM Buildbot has detected a new failure on builder  Full details are available at: https://lab.llvm.org/buildbot/#/builders/59/builds/26474 Here is the relevant piece of the build log for the reference | 
    
| 
           LLVM Buildbot has detected a new failure on builder  Full details are available at: https://lab.llvm.org/buildbot/#/builders/65/builds/24645 Here is the relevant piece of the build log for the reference | 
    
| 
           LLVM Buildbot has detected a new failure on builder  Full details are available at: https://lab.llvm.org/buildbot/#/builders/23/builds/15069 Here is the relevant piece of the build log for the reference | 
    
| 
           LLVM Buildbot has detected a new failure on builder  Full details are available at: https://lab.llvm.org/buildbot/#/builders/168/builds/16856 Here is the relevant piece of the build log for the reference | 
    
Add a macro __BPF_FEATURE_GOTOX for bpf target for cpu v4. So the developer can easily detect whether insn gotox is supported or not.
Add a macro __BPF_FEATURE_GOTOX for bpf target for cpu v4. So the developer can easily detect whether insn gotox is supported or not.
| 
           LLVM Buildbot has detected a new failure on builder  Full details are available at: https://lab.llvm.org/buildbot/#/builders/95/builds/18988 Here is the relevant piece of the build log for the reference | 
    
Add a macro __BPF_FEATURE_GOTOX for bpf target for cpu v4. So the developer can easily detect whether insn gotox is supported or not.