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

Fix clang vla warning #2736

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix clang vla warning #2736

wants to merge 1 commit into from

Conversation

cyyever
Copy link
Contributor

@cyyever cyyever commented Jun 15, 2024

Fix the error

/var/lib/jenkins/workspace/third_party/fbgemm/src/FbgemmI8Spmdm.cc:188:41: error: variable length arrays in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]

Copy link

netlify bot commented Jun 15, 2024

Deploy Preview for pytorch-fbgemm-docs ready!

Name Link
🔨 Latest commit 130b2f3
🔍 Latest deploy log https://app.netlify.com/sites/pytorch-fbgemm-docs/deploys/67afde6dbff8b800084f6b37
😎 Deploy Preview https://deploy-preview-2736--pytorch-fbgemm-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@cyyever cyyever changed the title Fix clang vla error Fix clang vla warning Oct 4, 2024
@jamesETsmith
Copy link

Thanks for working on this @cyyever. I'm still seeing the issue even when manually adding add the flags this PR implements (using clang/clang++ 18.1.8). Do you know if the fixs works for clang 18?

@cyyever
Copy link
Contributor Author

cyyever commented Feb 14, 2025

@jamesETsmith Can you help check compile_commands.json and make sure that the flag is indeed added to the problematic files' command lines?

@jamesETsmith
Copy link

@cyyever great idea, it looks like the flag is there (and in stdout during compilation):

From compile_commands.json:

{
  "directory": "/home/james.smith/apps/pytorch/build_qcog",
  "command": "/usr/bin/clang++ -DFBGEMM_STATIC -I/home/james.smith/apps/pytorch/third_party/cpuinfo/include -I/home/james.smith/apps/pytorch/third_party/fbgemm/third_party/asmjit/src -I/home/james.smith/apps/pytorch/third_party/fbgemm/include -I/home/james.smith/apps/pytorch/third_party/fbgemm -isystem /home/james.smith/apps/pytorch/third_party/protobuf/src -isystem /opt/intel/oneapi/mkl/latest/include -isystem /home/james.smith/apps/pytorch/third_party/XNNPACK/include -Wno-vla-cxx-extension -D_GLIBCXX_USE_CXX11_ABI=1 -fvisibility-inlines-hidden -DUSE_PTHREADPOOL -fopenmp=libomp -Wall -Wextra -Werror -Wno-deprecated-declarations -Wimplicit-fallthrough -g -std=c++17 -fPIC -fvisibility=hidden -DMKL_HAS_SBGEMM -DMKL_HAS_SHGEMM -o third_party/fbgemm/CMakeFiles/fbgemm_generic.dir/src/EmbeddingSpMDM.cc.o -c /home/james.smith/apps/pytorch/third_party/fbgemm/src/EmbeddingSpMDM.cc",
  "file": "/home/james.smith/apps/pytorch/third_party/fbgemm/src/EmbeddingSpMDM.cc",
  "output": "third_party/fbgemm/CMakeFiles/fbgemm_generic.dir/src/EmbeddingSpMDM.cc.o"
}

From STDOUT:

[26/2892] Building CXX object third_party/fbgemm/CMakeFiles/fbgemm_generic.dir/src/FbgemmI8Spmdm.cc.o
FAILED: third_party/fbgemm/CMakeFiles/fbgemm_generic.dir/src/FbgemmI8Spmdm.cc.o 
/usr/bin/ccache /usr/bin/clang++ -DFBGEMM_STATIC -I/home/james.smith/apps/pytorch/third_party/cpuinfo/include -I/home/james.smith/apps/pytorch/third_party/fbgemm/third_party/asmjit/src -I/home/james.smith/apps/pytorch/third_party/fbgemm/include -I/home/james.smith/apps/pytorch/third_party/fbgemm -isystem /home/james.smith/apps/pytorch/third_party/protobuf/src -isystem /opt/intel/oneapi/mkl/latest/include -isystem /home/james.smith/apps/pytorch/third_party/XNNPACK/include -Wno-vla-cxx-extension -D_GLIBCXX_USE_CXX11_ABI=1 -fvisibility-inlines-hidden -DUSE_PTHREADPOOL -fopenmp=libomp -Wall -Wextra -Werror -Wno-deprecated-declarations -Wimplicit-fallthrough -g -std=c++17 -fPIC -fvisibility=hidden -DMKL_HAS_SBGEMM -DMKL_HAS_SHGEMM -MD -MT third_party/fbgemm/CMakeFiles/fbgemm_generic.dir/src/FbgemmI8Spmdm.cc.o -MF third_party/fbgemm/CMakeFiles/fbgemm_generic.dir/src/FbgemmI8Spmdm.cc.o.d -o third_party/fbgemm/CMakeFiles/fbgemm_generic.dir/src/FbgemmI8Spmdm.cc.o -c /home/james.smith/apps/pytorch/third_party/fbgemm/src/FbgemmI8Spmdm.cc
/home/james.smith/apps/pytorch/third_party/fbgemm/src/FbgemmI8Spmdm.cc:86:32: error: variable length arrays in C++ are a Clang extension [-Werror,-Wvla-cxx-extension]
   86 |   alignas(64) uint8_t A_buffer[K * 32];

@cyyever
Copy link
Contributor Author

cyyever commented Feb 15, 2025

@jamesETsmith That flag is overwritten by '-Wall -Wextra -Werror' later. That means there is a flag order problem. Check where you added the flag and move it behind Wall

@jamesETsmith
Copy link

Thanks for the heads up @cyyever. I'm building FBGEMM through PyTorch and it doesn't seem like I can change the ordering of the CMAKE_CXX_FLAGS from the command line without modifying the cmake source like you have for FBGEMM or PyTorch. I replicated the changes from your pytorch PR and that seems to have worked.

Thanks for the work on this, I hope it gets merged.

@facebook-github-bot
Copy link
Contributor

@q10 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants