Skip to content

Reapply "[libc] build with -Werror (#73966)" #74506

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jan 8, 2024
Merged

Reapply "[libc] build with -Werror (#73966)" #74506

merged 1 commit into from
Jan 8, 2024

Conversation

nickdesaulniers
Copy link
Member

This reverts commit 6886a52.

Most of the errors observed in postsubmit have been addressed. We can
fix-forward the remaining ones.

Link: https://lab.llvm.org/buildbot/#/changes/117129

@llvmbot llvmbot added the libc label Dec 5, 2023
@llvmbot
Copy link
Member

llvmbot commented Dec 5, 2023

@llvm/pr-subscribers-libc

Author: Nick Desaulniers (nickdesaulniers)

Changes

This reverts commit 6886a52.

Most of the errors observed in postsubmit have been addressed. We can
fix-forward the remaining ones.

Link: https://lab.llvm.org/buildbot/#/changes/117129

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

2 Files Affected:

  • (modified) libc/cmake/modules/LLVMLibCObjectRules.cmake (+4)
  • (modified) libc/docs/dev/code_style.rst (+8)
diff --git a/libc/cmake/modules/LLVMLibCObjectRules.cmake b/libc/cmake/modules/LLVMLibCObjectRules.cmake
index 5fbbfd58db2d0..ef654bd7b34ab 100644
--- a/libc/cmake/modules/LLVMLibCObjectRules.cmake
+++ b/libc/cmake/modules/LLVMLibCObjectRules.cmake
@@ -43,6 +43,10 @@ function(_get_common_compile_options output_var flags)
     list(APPEND compile_options "-fno-rtti")
     list(APPEND compile_options "-Wall")
     list(APPEND compile_options "-Wextra")
+    # -DLIBC_WNO_ERROR=ON if you can't build cleanly with -Werror.
+    if(NOT LIBC_WNO_ERROR)
+      list(APPEND compile_options "-Werror")
+    endif()
     list(APPEND compile_options "-Wconversion")
     list(APPEND compile_options "-Wno-sign-conversion")
     list(APPEND compile_options "-Wimplicit-fallthrough")
diff --git a/libc/docs/dev/code_style.rst b/libc/docs/dev/code_style.rst
index a050a4c1d3dd7..eeeced0359adb 100644
--- a/libc/docs/dev/code_style.rst
+++ b/libc/docs/dev/code_style.rst
@@ -178,3 +178,11 @@ these functions do not call the constructors and destructors of the
 allocated/deallocated objects. So, use these functions carefully and only
 when it is absolutely clear that constructor and destructor invocation is
 not required.
+
+Warnings in sources
+===================
+
+We expect contributions to be free of warnings from the `minimum supported
+compiler versions`__ (and newer).
+
+.. __: https://libc.llvm.org/compiler_support.html#minimum-supported-versions

@nickdesaulniers nickdesaulniers marked this pull request as draft December 5, 2023 18:14
This was referenced Dec 6, 2023
Closed
Closed
@nickdesaulniers nickdesaulniers mentioned this pull request Jan 3, 2024
Closed
@nickdesaulniers
Reapply "[libc] build with -Werror (llvm#73966)"
fa4b0d1 
This reverts commit 6886a52.

Most of the errors observed in postsubmit have been addressed. We can
fix-forward the remaining ones.

Link: https://lab.llvm.org/buildbot/#/changes/117129
@nickdesaulniers nickdesaulniers marked this pull request as ready for review January 8, 2024 16:53
@nickdesaulniers
Copy link
Member Author

going to land this and keep an eye on the build bots.

@nickdesaulniers nickdesaulniers merged commit c52b467 into llvm:main Jan 8, 2024
@nickdesaulniers nickdesaulniers deleted the Werror2 branch January 8, 2024 17:07
@nickdesaulniers
Copy link
Member Author

tracking build failures. Going to see if we can fix-forward these.

@nickdesaulniers nickdesaulniers mentioned this pull request Jan 8, 2024
Merged
nickdesaulniers added a commit that referenced this pull request Jan 8, 2024
@nickdesaulniers
[libc] fix -Wmissing-braces (#77345)
c1023c5 
Fixes the following errors observed on the aarch64 fullbuild:


/home/libc-buildbot/libc-aarch64-ubuntu/libc-aarch64-ubuntu-fullbuild-dbg/llvm-project/libc/src/__support/HashTable/generic/bitmask_impl.inc:116:13:
    error: suggest braces around initialization of subobject
    [-Werror,-Wmissing-braces]
return {static_cast<bitmask_t>(mask_available().word ^
repeat_byte(0x80))};
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
{ }
    In file included from

/home/libc-buildbot/libc-aarch64-ubuntu/libc-aarch64-ubuntu-fullbuild-dbg/llvm-project/libc/src/search/hdestroy.cpp:10:

/home/libc-buildbot/libc-aarch64-ubuntu/libc-aarch64-ubuntu-fullbuild-dbg/llvm-project/libc/src/__support/HashTable/table.h:336:41:
    error: suggest braces around initialization of subobject
    [-Werror,-Wmissing-braces]
      iterator end() const { return {0, 0, {0}, *this}; }
                                            ^
                                            {}

Link:
https://lab.llvm.org/buildbot/#/builders/223/builds/33868/steps/6/logs/stdio
Link: #74506
@nickdesaulniers nickdesaulniers mentioned this pull request Jan 8, 2024
Merged
nickdesaulniers added a commit that referenced this pull request Jan 8, 2024
@nickdesaulniers
[libc] fix more -Wmissing-brace (#77382)
f700d74 
Similar to #77345, the buildbots are observing similar warnings for the
sse2
implementation.

llvm-project/libc/src/__support/HashTable/sse2/bitmask_impl.inc:36:13:
    error: suggest braces around initialization of subobject
    [-Werror,-Wmissing-braces]
    return {bitmask};
            ^~~~~~~
            {      }
llvm-project/libc/src/__support/HashTable/sse2/bitmask_impl.inc:45:13:
    error: suggest braces around initialization of subobject
    [-Werror,-Wmissing-braces]
    return {static_cast<uint16_t>(~mask_available().word)};
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            {                                            }

Link:
https://lab.llvm.org/buildbot/#/builders/163/builds/49350/steps/8/logs/stdio
Link: #74506
@nickdesaulniers nickdesaulniers mentioned this pull request Jan 8, 2024
Merged
nickdesaulniers added a commit that referenced this pull request Jan 9, 2024
@nickdesaulniers
[libc] fix -Wconversion (#77384)
6958986 
Fixes the following from GCC:

    llvm-project/libc/src/string/memory_utils/op_x86.h:236:24: error:
conversion from ‘long unsigned int’ to ‘uint32_t’ {aka ‘unsigned int’}
may
    change value [-Werror=conversion]
      236 |   return (xored >> 32) | (xored & 0xFFFFFFFF);
          |          ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~

Link:
https://lab.llvm.org/buildbot/#/builders/250/builds/16236/steps/8/logs/stdio
Link: #74506
@nickdesaulniers nickdesaulniers mentioned this pull request Jan 11, 2024
Merged
nickdesaulniers added a commit to nickdesaulniers/llvm-project that referenced this pull request Jan 11, 2024
@nickdesaulniers
[libc][cmake] reset COMPILE_DEFINITIONS
32b592a 
While trying to enable -Werror (llvm#74506), the 32b ARM build bot reported an
error stemming from -Wshorten-64-to-32 related to usages of `off_t`.

I failed to fix these properly in llvm#77350 (the 32b ARM build is not a fullbuild)
and llvm#77396.

It turns out, the preprocessor defines `-D_LARGEFILE_SOURCE` and
`-D_FILE_OFFSET_BITS=64` were being set for llvmlibc when using the cmake build
system. In particular, these preprocessor defines are feature test macros used
by glibc, and which have effects no the corresponding ABI for types like
`off_t` (for instance, should `off_t` be 32b or 64b on 32b targets).

But who was setting these?  Turns out that the use of add_compile_definitions
in llvm/cmake/modules/HandleLLVMOptions.cmake was setting these (and more),
which is then inherited by every subdirectory.  While some of these defines
maybe make sense for host builds, they do not make sense for libraries for the
target. The full list of defines being set prior to this commit:

- `-D_GNU_SOURCE`
- `-D_FILE_OFFSET_BITS=64`
- `-D_DEBUG`
- `-D_GLIBCXX_ASSERTIONS`
- `-D_LARGEFILE_SOURCE`
- `-D_FILE_OFFSET_BITS=64`
- `-D__STDC_CONSTANT_MACROS`
- `-D__STDC_FORMAT_MACROS`
- `-D__STDC_LIMIT_MACROS`

If we desire any of the above, we should manually reset them.

Fix this by resetting COMPILE_DEFINITIONS for the libc/ subdirectory.

Side note: to debug 'directory properties' in cmake, you first need to use
`get_directory_property` to fetch the corresponding value into a variable
first, then that variable can be printed via `message`.

Link: https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS
Link: https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS

Fixes: llvm#77395
nickdesaulniers added a commit that referenced this pull request Jan 16, 2024
@nickdesaulniers
[libc][cmake] reset COMPILE_DEFINITIONS (#77810)
f1f1875 
While trying to enable -Werror (#74506), the 32b ARM build bot reported
an
error stemming from -Wshorten-64-to-32 related to usages of `off_t`.

I failed to fix these properly in #77350 (the 32b ARM build is not a
fullbuild)
and #77396.

It turns out, the preprocessor defines `-D_LARGEFILE_SOURCE` and
`-D_FILE_OFFSET_BITS=64` were being set for llvmlibc when using the
cmake build
system. In particular, these preprocessor defines are feature test
macros used
by glibc, and which have effects no the corresponding ABI for types like
`off_t` (for instance, should `off_t` be 32b or 64b on 32b targets).

But who was setting these? Turns out that the use of
add_compile_definitions
in llvm/cmake/modules/HandleLLVMOptions.cmake was setting these (and
more),
which is then inherited by every subdirectory. While some of these
defines
maybe make sense for host builds, they do not make sense for libraries
for the
target. The full list of defines being set prior to this commit:

- `-D_GNU_SOURCE`
- `-D_FILE_OFFSET_BITS=64`
- `-D_DEBUG`
- `-D_GLIBCXX_ASSERTIONS`
- `-D_LARGEFILE_SOURCE`
- `-D_FILE_OFFSET_BITS=64`
- `-D__STDC_CONSTANT_MACROS`
- `-D__STDC_FORMAT_MACROS`
- `-D__STDC_LIMIT_MACROS`

If we desire any of the above, we should manually reset them.

Fix this by resetting COMPILE_DEFINITIONS for the libc/ subdirectory.

Side note: to debug 'directory properties' in cmake, you first need to
use
`get_directory_property` to fetch the corresponding value into a
variable
first, then that variable can be printed via `message`.

Link:
https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS
Link:
https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS

Fixes: #77395
@nickdesaulniers nickdesaulniers mentioned this pull request Jan 19, 2024
Merged
nickdesaulniers added a commit to nickdesaulniers/llvm-project that referenced this pull request Jan 19, 2024
@nickdesaulniers
[libc] remove extra -Werror
01a28d8 
-Werror is now a global default as of
commit c52b467 ("Reapply "[libc] build with -Werror (llvm#73966)" (llvm#74506)")
nickdesaulniers added a commit that referenced this pull request Jan 19, 2024
@nickdesaulniers
[libc] remove extra -Werror (#78761)
2c0d206 
-Werror is now a global default as of
commit c52b467 ("Reapply "[libc] build with -Werror (#73966)"
(#74506)")
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
@nickdesaulniers @justinfargnoli
Reapply "[libc] build with -Werror (llvm#73966)" (llvm#74506)
ca1ebc4 
This reverts commit 6886a52.

Most of the errors observed in postsubmit have been addressed. We can
fix-forward the remaining ones.

Link: https://lab.llvm.org/buildbot/#/changes/117129
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
@nickdesaulniers @justinfargnoli
[libc] fix -Wmissing-braces (llvm#77345)
23f585c 
Fixes the following errors observed on the aarch64 fullbuild:


/home/libc-buildbot/libc-aarch64-ubuntu/libc-aarch64-ubuntu-fullbuild-dbg/llvm-project/libc/src/__support/HashTable/generic/bitmask_impl.inc:116:13:
    error: suggest braces around initialization of subobject
    [-Werror,-Wmissing-braces]
return {static_cast<bitmask_t>(mask_available().word ^
repeat_byte(0x80))};
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
{ }
    In file included from

/home/libc-buildbot/libc-aarch64-ubuntu/libc-aarch64-ubuntu-fullbuild-dbg/llvm-project/libc/src/search/hdestroy.cpp:10:

/home/libc-buildbot/libc-aarch64-ubuntu/libc-aarch64-ubuntu-fullbuild-dbg/llvm-project/libc/src/__support/HashTable/table.h:336:41:
    error: suggest braces around initialization of subobject
    [-Werror,-Wmissing-braces]
      iterator end() const { return {0, 0, {0}, *this}; }
                                            ^
                                            {}

Link:
https://lab.llvm.org/buildbot/#/builders/223/builds/33868/steps/6/logs/stdio
Link: llvm#74506
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
@nickdesaulniers @justinfargnoli
[libc] fix more -Wmissing-brace (llvm#77382)
cea22fd 
Similar to llvm#77345, the buildbots are observing similar warnings for the
sse2
implementation.

llvm-project/libc/src/__support/HashTable/sse2/bitmask_impl.inc:36:13:
    error: suggest braces around initialization of subobject
    [-Werror,-Wmissing-braces]
    return {bitmask};
            ^~~~~~~
            {      }
llvm-project/libc/src/__support/HashTable/sse2/bitmask_impl.inc:45:13:
    error: suggest braces around initialization of subobject
    [-Werror,-Wmissing-braces]
    return {static_cast<uint16_t>(~mask_available().word)};
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            {                                            }

Link:
https://lab.llvm.org/buildbot/#/builders/163/builds/49350/steps/8/logs/stdio
Link: llvm#74506
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
@nickdesaulniers @justinfargnoli
[libc] fix -Wconversion (llvm#77384)
660c147 
Fixes the following from GCC:

    llvm-project/libc/src/string/memory_utils/op_x86.h:236:24: error:
conversion from ‘long unsigned int’ to ‘uint32_t’ {aka ‘unsigned int’}
may
    change value [-Werror=conversion]
      236 |   return (xored >> 32) | (xored & 0xFFFFFFFF);
          |          ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~

Link:
https://lab.llvm.org/buildbot/#/builders/250/builds/16236/steps/8/logs/stdio
Link: llvm#74506
justinfargnoli pushed a commit to justinfargnoli/llvm-project that referenced this pull request Jan 28, 2024
@nickdesaulniers @justinfargnoli
[libc][cmake] reset COMPILE_DEFINITIONS (llvm#77810)
512534a 
While trying to enable -Werror (llvm#74506), the 32b ARM build bot reported
an
error stemming from -Wshorten-64-to-32 related to usages of `off_t`.

I failed to fix these properly in llvm#77350 (the 32b ARM build is not a
fullbuild)
and llvm#77396.

It turns out, the preprocessor defines `-D_LARGEFILE_SOURCE` and
`-D_FILE_OFFSET_BITS=64` were being set for llvmlibc when using the
cmake build
system. In particular, these preprocessor defines are feature test
macros used
by glibc, and which have effects no the corresponding ABI for types like
`off_t` (for instance, should `off_t` be 32b or 64b on 32b targets).

But who was setting these? Turns out that the use of
add_compile_definitions
in llvm/cmake/modules/HandleLLVMOptions.cmake was setting these (and
more),
which is then inherited by every subdirectory. While some of these
defines
maybe make sense for host builds, they do not make sense for libraries
for the
target. The full list of defines being set prior to this commit:

- `-D_GNU_SOURCE`
- `-D_FILE_OFFSET_BITS=64`
- `-D_DEBUG`
- `-D_GLIBCXX_ASSERTIONS`
- `-D_LARGEFILE_SOURCE`
- `-D_FILE_OFFSET_BITS=64`
- `-D__STDC_CONSTANT_MACROS`
- `-D__STDC_FORMAT_MACROS`
- `-D__STDC_LIMIT_MACROS`

If we desire any of the above, we should manually reset them.

Fix this by resetting COMPILE_DEFINITIONS for the libc/ subdirectory.

Side note: to debug 'directory properties' in cmake, you first need to
use
`get_directory_property` to fetch the corresponding value into a
variable
first, then that variable can be printed via `message`.

Link:
https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS
Link:
https://www.gnu.org/software/libc/manual/html_node/Feature-Test-Macros.html#index-_005fFILE_005fOFFSET_005fBITS

Fixes: llvm#77395
qihangkong pushed a commit to rvgpu/llvm that referenced this pull request Apr 18, 2024
@nickdesaulniers
[libc] fix more -Wmissing-brace
8b96859 
Similar to #77345, the buildbots are observing similar warnings for the sse2
implementation.

    llvm-project/libc/src/__support/HashTable/sse2/bitmask_impl.inc:36:13:
    error: suggest braces around initialization of subobject
    [-Werror,-Wmissing-braces]
    return {bitmask};
            ^~~~~~~
            {      }
    llvm-project/libc/src/__support/HashTable/sse2/bitmask_impl.inc:45:13:
    error: suggest braces around initialization of subobject
    [-Werror,-Wmissing-braces]
    return {static_cast<uint16_t>(~mask_available().word)};
            ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
            {                                            }

Link: https://lab.llvm.org/buildbot/#/builders/163/builds/49350/steps/8/logs/stdio
Link: llvm/llvm-project#74506
qihangkong pushed a commit to rvgpu/llvm that referenced this pull request Apr 18, 2024
@nickdesaulniers
[libc] fix -Wconversion
546b0db 
Fixes the following from GCC:

    llvm-project/libc/src/string/memory_utils/op_x86.h:236:24: error:
    conversion from ‘long unsigned int’ to ‘uint32_t’ {aka ‘unsigned int’} may
    change value [-Werror=conversion]
      236 |   return (xored >> 32) | (xored & 0xFFFFFFFF);
          |          ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~

Link: https://lab.llvm.org/buildbot/#/builders/250/builds/16236/steps/8/logs/stdio
Link: llvm/llvm-project#74506
qihangkong pushed a commit to rvgpu/llvm that referenced this pull request Apr 18, 2024
@nickdesaulniers
[libc] fix -Wmissing-braces
0cf83d9 
Fixes the following errors observed on the aarch64 fullbuild:

    /home/libc-buildbot/libc-aarch64-ubuntu/libc-aarch64-ubuntu-fullbuild-dbg/llvm-project/libc/src/__support/HashTable/generic/bitmask_impl.inc:116:13:
    error: suggest braces around initialization of subobject
    [-Werror,-Wmissing-braces]
        return {static_cast<bitmask_t>(mask_available().word ^ repeat_byte(0x80))};
                ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                {                                                                }
    In file included from
    /home/libc-buildbot/libc-aarch64-ubuntu/libc-aarch64-ubuntu-fullbuild-dbg/llvm-project/libc/src/search/hdestroy.cpp:10:
    /home/libc-buildbot/libc-aarch64-ubuntu/libc-aarch64-ubuntu-fullbuild-dbg/llvm-project/libc/src/__support/HashTable/table.h:336:41:
    error: suggest braces around initialization of subobject
    [-Werror,-Wmissing-braces]
      iterator end() const { return {0, 0, {0}, *this}; }
                                            ^
                                            {}

Link: https://lab.llvm.org/buildbot/#/builders/223/builds/33868/steps/6/logs/stdio
Link: llvm/llvm-project#74506
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lntue lntue lntue approved these changes

@michaelrj-google michaelrj-google Awaiting requested review from michaelrj-google

@gchatelet gchatelet Awaiting requested review from gchatelet

@legrosbuffle legrosbuffle Awaiting requested review from legrosbuffle

@jhuber6 jhuber6 Awaiting requested review from jhuber6

Assignees
No one assigned
Labels
libc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nickdesaulniers @llvmbot @lntue