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

hurd: Fix build with -Werror,-Wswitch #78520

Closed
wants to merge 1 commit into from
Closed

Conversation

sthibaul
Copy link
Contributor

We do not handle all architectures, llvm_unreachable is called after the switch, so we can just break.

Fixes https://lab.llvm.org/buildbot/#/builders/19/builds/23702

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Jan 17, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Jan 17, 2024

@llvm/pr-subscribers-clang-driver

@llvm/pr-subscribers-clang

Author: Samuel Thibault (sthibaul)

Changes

We do not handle all architectures, llvm_unreachable is called after the switch, so we can just break.

Fixes https://lab.llvm.org/buildbot/#/builders/19/builds/23702


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

1 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/Hurd.cpp (+2)
diff --git a/clang/lib/Driver/ToolChains/Hurd.cpp b/clang/lib/Driver/ToolChains/Hurd.cpp
index 5074eda5f41559..36499fb11ad88f 100644
--- a/clang/lib/Driver/ToolChains/Hurd.cpp
+++ b/clang/lib/Driver/ToolChains/Hurd.cpp
@@ -135,6 +135,8 @@ Tool *Hurd::buildAssembler() const {
 
 std::string Hurd::getDynamicLinker(const ArgList &Args) const {
   switch (getArch()) {
+  default:
+    break;
   case llvm::Triple::x86:
     return "/lib/ld.so";
   case llvm::Triple::x86_64:

@fmayer
Copy link
Contributor

fmayer commented Jan 18, 2024

Drive-by: is the llvm_unreachable actually unreachable? I.e. we never call this function with this case? Otherwise we are introducing UB in NDEBUG builds

/// In NDEBUG builds, if the platform does not support a builtin unreachable
/// then we call an internal LLVM runtime function. Otherwise the behavior is
/// controlled by the CMake flag
///   -DLLVM_UNREACHABLE_OPTIMIZE
/// * When "ON" (default) llvm_unreachable() becomes an optimizer hint
///   that the current location is not supposed to be reachable: the hint
///   turns such code path into undefined behavior.  On compilers that don't
///   support such hints, prints a reduced message instead and aborts the
///   program.

@sthibaul
Copy link
Contributor Author

Drive-by: is the llvm_unreachable actually unreachable? I.e. we never call this function with this case?

Hurd triplets are only defined for x86 and x86_64.

Copy link
Contributor

@fmayer fmayer left a comment

Choose a reason for hiding this comment

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

Is there a reason you put this first? Otherwise IMO it is slightly neater to put it last.

We do not handle all architectures, llvm_unreachable is called after the
switch, so we can just break.

Fixes https://lab.llvm.org/buildbot/#/builders/19/builds/23702
@sthibaul
Copy link
Contributor Author

Is there a reason you put this first?

No particular reason.

Otherwise IMO it is slightly neater to put it last.

Done so.

@DamonFool
Copy link
Member

Is there a reason you put this first? Otherwise IMO it is slightly neater to put it last.

This bug had been fixed here:

commit 1b60ddd920e0caadfa85cc7013b559d6453d7e3e
Author: Jie Fu <jiefu@tencent.com>
Date:   Thu Jan 18 07:34:09 2024 +0800

    [Hurd] Fix -Wswitch in Hurd::getDynamicLinker (NFC)
    
    llvm-project/clang/lib/Driver/ToolChains/Hurd.cpp:137:11:
     error: 60 enumeration values not handled in switch: 'UnknownArch', 'arm', 'armeb'... [-Werror,-Wswitch]
      switch (getArch()) {
              ^~~~~~~~~
    1 error generated.

Thanks.

@sthibaul sthibaul closed this Jan 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants