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.

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

Fix build with clang v18 [-Wvla-cxx-extension] #1813

Closed
wants to merge 3 commits into from
Closed

Fix build with clang v18 [-Wvla-cxx-extension] #1813

wants to merge 3 commits into from

Conversation

kinkie
Copy link
Contributor

@kinkie kinkie commented May 18, 2024 •
edited by rousskov 

src/fs/rock/RockRebuild.cc:356:17: error: variable length arrays
in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]
    char hdrBuf[SwapDir::HeaderSize];
note: initializer of 'HeaderSize' is unknown

@kinkie
RockStore: do not use variable-length arrays
6ca4fa4 
Variable length arrays are legal in C, but not
part of the C++ standard.
Current clang versions emit a warning if
these are used, failing the build in Rock.
@kinkie
Copy link
Contributor Author

kinkie commented May 18, 2024 •
edited

Landing this PR is a requirement for us to start CI testing on
Fedora 40, current Fedora Rawhide, current OpenSUSE Tumbleweed,
Ubuntu 24.04 Noble, and possibly more

rousskov
rousskov reviewed May 18, 2024
View reviewed changes
Copy link
Contributor

@rousskov rousskov left a comment

Choose a reason for hiding this comment

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

Please share clang version and the warning text.

@kinkie
Copy link
Contributor Author

kinkie commented May 18, 2024 •
edited

Versions:

Checking on fedora-40
clang version 18.1.1 (Fedora 18.1.1-1.fc40)
Target: x86_64-redhat-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
Configuration file: /etc/clang/clang.cfg
Checking on fedora-rawhide
clang version 18.1.4 (Fedora 18.1.4-2.fc41)
Target: x86_64-redhat-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
Configuration file: /etc/clang/clang.cfg
Checking on opensuse-tumbleweed
clang version 18.1.5
Target: x86_64-suse-linux
Thread model: posix
InstalledDir: /usr/bin
Checking on ubuntu-noble
Ubuntu clang version 18.1.3 (1)
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin

From failed build attempt:

libtool: compile:  clang++ -std=c++17 -std=c++17 -DHAVE_CONFIG_H -DDEFAULT_CONFIG_FILE=\"/srv/jenkins/workspace/anybranch-anyarch-matrix/matrix/aarch64/fedora-40/clang/layer-02-maximus/btlayer-02-maximus/squid-7.0.0-VCS/_inst/etc/squid.conf\" -DDEFAULT_SQUID_DATA_DIR=\"/srv/jenkins/workspace/anybranch-anyarch-matrix/matrix/aarch64/fedora-40/clang/layer-02-maximus/btlayer-02-maximus/squid-7.0.0-VCS/_inst/share\" -DDEFAULT_SQUID_CONFIG_DIR=\"/srv/jenkins/workspace/anybranch-anyarch-matrix/matrix/aarch64/fedora-40/clang/layer-02-maximus/btlayer-02-maximus/squid-7.0.0-VCS/_inst/etc\" -I../../../.. -I../../../../include -I../../../../lib -I../../../../src -I../../include -I../../../../src/fs -I/usr/include/p11-kit-1 -I/usr/include/libxml2 -DWITH_GZFILEOP -Wall -Wextra -Wno-unused-private-field -Wno-implicit-fallthrough -Wpointer-arith -Wwrite-strings -Wcomments -Wshadow -Wmissing-declarations -Woverloaded-virtual -Werror -D_REENTRANT -g -O2 -march=native -MT rock/RockRebuild.lo -MD -MP -MF rock/.deps/RockRebuild.Tpo -c ../../../../src/fs/rock/RockRebuild.cc  -fPIC -DPIC -o rock/.libs/RockRebuild.o
../../../../src/fs/rock/RockRebuild.cc:356:17: error: variable length arrays in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]
  356 |     char hdrBuf[SwapDir::HeaderSize];
      |                 ^~~~~~~~~~~~~~~~~~~
../../../../src/fs/rock/RockRebuild.cc:356:17: note: initializer of 'HeaderSize' is unknown
../../../../src/fs/rock/RockSwapDir.h:151:26: note: declared here
  151 |     static const int64_t HeaderSize; ///< on-disk db header size
      |                          ^
1 error generated.
make[4]: *** [Makefile:996: rock/RockRebuild.lo] Error 1
make[4]: *** Waiting for unfinished jobs....

@rousskov rousskov changed the title RockStore: do not use variable-length arrays Fix build with clang v18 [-Wvla-cxx-extension] May 18, 2024
@rousskov
fixup: Reduce PR changes
9edef1f 
We probably want to switch to constexpr in some contexts (possibly
including this one) but not others. And that switching may require other
adjustments (e.g., removing "static"). Let's not bring that separate
issue into this PR.
rousskov
rousskov previously approved these changes May 18, 2024
View reviewed changes
Copy link
Contributor

@rousskov rousskov left a comment •
edited

Choose a reason for hiding this comment

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

Thank you for providing information about the error and fixing the list of affected environments. I moved that info into PR title/description and used it to make one small code adjustment (see commit 9edef1f for details) which I have tested with clang v18.

If you are OK with the current PR state, let's merge this fix!

@rousskov rousskov added S-waiting-for-author author action is expected (and usually required) S-could-use-an-approval An approval may speed this PR merger (but is not required) labels May 18, 2024
@kinkie kinkie added M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels backport-to-v6 maintainer has approved these changes for v6 backporting and removed S-waiting-for-author author action is expected (and usually required) labels May 18, 2024
@yadij
Copy link
Contributor

yadij commented May 19, 2024

@rousskov, fixed whitespace - please redo your approval to fast-track

@yadij yadij requested review from rousskov and removed request for rousskov May 19, 2024 07:46
@rousskov rousskov removed the S-could-use-an-approval An approval may speed this PR merger (but is not required) label May 19, 2024
@rousskov
Copy link
Contributor 

07:56:41 FATAL: Unable to produce a script file

In hope to work around a seemingly spurious Jenkins failure, I am asking Jenkins to retest this please.

@kinkie
Copy link
Contributor Author

kinkie commented May 19, 2024 


07:56:41 FATAL: Unable to produce a script file

In hope to work around a seemingly spurious Jenkins failure, I am asking Jenkins to retest this please.

Disk full on a node. On it

squid-anubis pushed a commit that referenced this pull request May 19, 2024
@kinkie @squid-anubis
Fix build with clang v18 [-Wvla-cxx-extension] (#1813)
dbbd46d 
    src/fs/rock/RockRebuild.cc:356:17: error: variable length arrays
    in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]
        char hdrBuf[SwapDir::HeaderSize];
    note: initializer of 'HeaderSize' is unknown
@squid-anubis squid-anubis added the M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels label May 19, 2024
@squid-anubis squid-anubis added M-merged https://github.com/measurement-factory/anubis#pull-request-labels and removed M-waiting-staging-checks https://github.com/measurement-factory/anubis#pull-request-labels M-cleared-for-merge https://github.com/measurement-factory/anubis#pull-request-labels labels May 20, 2024
squidadm pushed a commit to squidadm/squid that referenced this pull request May 20, 2024
@kinkie @squidadm
Fix build with clang v18 [-Wvla-cxx-extension] (squid-cache#1813)
55ef61b 
    src/fs/rock/RockRebuild.cc:356:17: error: variable length arrays
    in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]
        char hdrBuf[SwapDir::HeaderSize];
    note: initializer of 'HeaderSize' is unknown
@squidadm squidadm removed the backport-to-v6 maintainer has approved these changes for v6 backporting label May 20, 2024
@squidadm
Copy link
Collaborator

queued for backport to v6

kinkie added a commit that referenced this pull request May 21, 2024
@squidadm @kinkie
Fix build with clang v18 [-Wvla-cxx-extension] (#1813) (#1817)
fb0d181 
src/fs/rock/RockRebuild.cc:356:17: error: variable length arrays
    in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]
        char hdrBuf[SwapDir::HeaderSize];
    note: initializer of 'HeaderSize' is unknown

Co-authored-by: Francesco Chemolli <5175948+kinkie@users.noreply.github.com>
kinkie added a commit to kinkie/squid that referenced this pull request Jun 9, 2024
@kinkie
Fix build with clang v18 [-Wvla-cxx-extension] (squid-cache#1813)
0189e42 
    src/fs/rock/RockRebuild.cc:356:17: error: variable length arrays
    in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]
        char hdrBuf[SwapDir::HeaderSize];
    note: initializer of 'HeaderSize' is unknown
@kinkie kinkie mentioned this pull request Jun 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rousskov rousskov rousskov approved these changes

@yadij yadij yadij approved these changes

Assignees
No one assigned
Labels
M-merged https://github.com/measurement-factory/anubis#pull-request-labels
Projects
None yet
Milestone
No milestone
5 participants
@kinkie @yadij @rousskov @squidadm @squid-anubis