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

[style] Clean up warnings in code compilation #2383

Merged
merged 1 commit into from
May 4, 2023

Conversation

MizukiCry
Copy link
Contributor

@MizukiCry MizukiCry commented Apr 8, 2023

What problem does this PR solve?

Fix some warnings in code compilation.

This PR is a subtask of #1264. Due to the number of warnings, this task will be divided into several PRs.

OS: Debian GNU/Linux 9 (stretch) x86_64
GCC: gcc (Debian 6.3.0-18+deb9u1) 6.3.0 20170516

Fixed modules:

  • //curvefs/...
  • //test/...
  • //src/...
  • //proto/...
  • //nebd/...
  • //include/...
  • //tools/...

Fixed warnings:

  • [-Wunused-parameter]
  • [-Wsign-compare]
  • [-Wnarrowing]
  • [-Wreorder]
  • [-Wunused-variable]
  • [-Wvla]
  • [-Wmissing-field-initializers]
  • [-Wwrite-strings]
  • [-Wunused-but-set-variable]
  • [-Wdeprecated-declarations]
  • [-Wformat=]
  • [-Wmaybe-uninitialized]

Build result (Ignored submodule and external warnings):

root@be2939e66c7d:~/curve# bazel clean
INFO: Starting clean (this may take a while). Consider using --async if the clean takes more than several minutes.
root@be2939e66c7d:~/curve# bazel build //...

INFO: Analyzed 290 targets (190 packages loaded, 5106 targets configured).
INFO: Found 290 targets...
INFO: Elapsed time: 1277.986s, Critical Path: 37.67s
INFO: 3491 processes: 794 internal, 2697 processwrapper-sandbox.
INFO: Build completed successfully, 3491 total actions
INFO: Build completed successfully, 3491 total actions

Issue Number: #1264

Problem Summary:

What is changed and how it works?

What's Changed: Relevant codes that cause warnings.

How it Works: By correcting corresponding codes.

Side effects(Breaking backward compatibility? Performance regression?): This may cause unknown bugs.

Check List

  • Relevant documentation/comments are changed or added
  • I acknowledge that all my contributions will be made under the project's license

@MizukiCry
Copy link
Contributor Author

cicheck

5 similar comments
@MizukiCry
Copy link
Contributor Author

cicheck

@MizukiCry
Copy link
Contributor Author

cicheck

@MizukiCry
Copy link
Contributor Author

cicheck

@MizukiCry
Copy link
Contributor Author

cicheck

@MizukiCry
Copy link
Contributor Author

cicheck

@ilixiaocui
Copy link
Contributor

image

It is recommended to merge into one commit submission.

When you have a problem with ci, you can click to check it in detail.

@MizukiCry MizukiCry force-pushed the warning-clean-up branch 3 times, most recently from 69b3273 to 8faca0e Compare April 10, 2023 09:28
@ilixiaocui
Copy link
Contributor

Maybe you can use push -f to push the code without turning it off.

@MizukiCry
Copy link
Contributor Author

Maybe you can use push -f to push the code without turning it off.

@ilixiaocui Sorry, could you please tell me how to merge these commits into one? I tried using git rebase -i but that also merged some commits by others, and I have no idea why the commits I made months before still exist here.

@ilixiaocui
Copy link
Contributor

Maybe you can use push -f to push the code without turning it off.

@ilixiaocui Sorry, could you please tell me how to merge these commits into one? I tried using git rebase -i but that also merged some commits by others, and I have no idea why the commits I made months before still exist here.

  1. use git reset commitid reset the commits related to the current compile.
  2. use git commit -s -m "commit message" to create a new commit.
  3. use git push -f your-repo-name your-branch-name to update your branch. pr will be automatically synchronized.

@MizukiCry
Copy link
Contributor Author

Maybe you can use push -f to push the code without turning it off.

@ilixiaocui Sorry, could you please tell me how to merge these commits into one? I tried using git rebase -i but that also merged some commits by others, and I have no idea why the commits I made months before still exist here.

  1. use git reset commitid reset the commits related to the current compile.
  2. use git commit -s -m "commit message" to create a new commit.
  3. use git push -f your-repo-name your-branch-name to update your branch. pr will be automatically synchronized.

I tried it and the commits made by others between my commits are also in the changed files. Is that correct?

@ilixiaocui
Copy link
Contributor

Maybe you can use push -f to push the code without turning it off.

@ilixiaocui Sorry, could you please tell me how to merge these commits into one? I tried using git rebase -i but that also merged some commits by others, and I have no idea why the commits I made months before still exist here.

  1. use git reset commitid reset the commits related to the current compile.
  2. use git commit -s -m "commit message" to create a new commit.
  3. use git push -f your-repo-name your-branch-name to update your branch. pr will be automatically synchronized.

I tried it and the commits made by others between my commits are also in the changed files. Is that correct?

Then you need to use git pull -r curve-repo curve-branch to update your local branch before doing the above steps.

Now, you can excute git pull -r curve-repo curve-branch and than git push -f your-repo-name your-branch-name. It should be able to correct the files modified by others in your commit.

@MizukiCry
Copy link
Contributor Author

Maybe you can use push -f to push the code without turning it off.

@ilixiaocui Sorry, could you please tell me how to merge these commits into one? I tried using git rebase -i but that also merged some commits by others, and I have no idea why the commits I made months before still exist here.

  1. use git reset commitid reset the commits related to the current compile.
  2. use git commit -s -m "commit message" to create a new commit.
  3. use git push -f your-repo-name your-branch-name to update your branch. pr will be automatically synchronized.

I tried it and the commits made by others between my commits are also in the changed files. Is that correct?

Then you need to use git pull -r curve-repo curve-branch to update your local branch before doing the above steps.

Now, you can excute git pull -r curve-repo curve-branch and than git push -f your-repo-name your-branch-name. It should be able to correct the files modified by others in your commit.

It works. Thanks!

@fansehep
Copy link
Member

Hi, @MizukiCry, Thanks for you contribution. Fixing compile warnings is troublesome things. You'd better divided into multiple modules and slowly repaired. One fix pull request per module is fine, otherwise I'm afraid you'll have a good battle with our CI.

@MizukiCry
Copy link
Contributor Author

Hi, @MizukiCry, Thanks for you contribution. Fixing compile warnings is troublesome things. You'd better divided into multiple modules and slowly repaired. One fix pull request per module is fine, otherwise I'm afraid you'll have a good battle with our CI.

Thanks for your advice! I'll try that in future coding.

@MizukiCry
Copy link
Contributor Author

cicheck

@MizukiCry MizukiCry marked this pull request as ready for review April 11, 2023 01:11
@MizukiCry MizukiCry changed the title [WIP] Clean up warnings in code compilation [style] Clean up warnings in code compilation Apr 11, 2023
@MizukiCry
Copy link
Contributor Author

@ilixiaocui Please help review this PR:).
I'll divide this task into several PRs.

@wuhongsong
Copy link
Contributor

@ilixiaocui take it

@Ziy1-Tan
Copy link
Contributor

Hey, @MizukiCry ! Maybe it's a good way to divide the PR by the binary target? so that you can check whether the binary under chunkserver folder produces warnings by e.g. bazel build //src//chunkserver//... :)

# curvebs
bazel query '//src/...'
# curvefs
bazel query '//curvefs/src/...'

@ilixiaocui
Copy link
Contributor

herwise I'm afraid you'll have a good battle with our CI.

Thank you for your contribution!

  1. Can you note which module and type of warning this PR mainly cleans up?
  2. Can you post the result of compiling this module?

@MizukiCry
Copy link
Contributor Author

Hey, @MizukiCry ! Maybe it's a good way to divide the PR by the binary target? so that you can check whether the binary under chunkserver folder produces warnings by e.g. bazel build //src//chunkserver//... :)

# curvebs
bazel query '//src/...'
# curvefs
bazel query '//curvefs/src/...'

Thanks, I got it! I'll try that next.

@MizukiCry
Copy link
Contributor Author

herwise I'm afraid you'll have a good battle with our CI.

Thank you for your contribution!

  1. Can you note which module and type of warning this PR mainly cleans up?
  2. Can you post the result of compiling this module?

I'm sorry that I just randomly correct some warnings. If it's necessary, I can retry cleaning warnings by modules.

@MizukiCry MizukiCry closed this Apr 12, 2023
@wu-hanqing wu-hanqing self-requested a review April 19, 2023 07:26
@wu-hanqing
Copy link
Contributor

Could someone please help review this pr?

Have you tried using Clang? the simplest way is CC=clang CXX=clang bazel build //...

@MizukiCry
Copy link
Contributor Author

Could someone please help review this pr?

Have you tried using Clang? the simplest way is CC=clang CXX=clang bazel build //...

I'm currently using docker for development, when I use clang I'll get some compile errors that seem caused by the environment, and I have no idea deal with it. So I only tried to compile directly.

@Cyber-SiKu
Copy link
Contributor

Could someone please help review this pr?

Have you tried using Clang? the simplest way is CC=clang CXX=clang bazel build //...

I'm currently using docker for development, when I use clang I'll get some compile errors that seem caused by the environment, and I have no idea deal with it. So I only tried to compile directly.

What are the main mistakes? You can post it.

@MizukiCry
Copy link
Contributor Author

Could someone please help review this pr?

Have you tried using Clang? the simplest way is CC=clang CXX=clang bazel build //...

I'm currently using docker for development, when I use clang I'll get some compile errors that seem caused by the environment, and I have no idea deal with it. So I only tried to compile directly.

What are the main mistakes? You can post it.

I created a new container and compile it with clang, then got:

# CC=clang CXX=clang bazel build //...
...
ERROR: /root/.cache/bazel/_bazel_root/e9e6a7354dba985d31e61d4d2962d0f8/external/com_google_protobuf/BUILD:354:10: Linking external/com_google_protobuf/protoc failed: (Exit 1): clang failed: error executing command /usr/bin/clang @bazel-out/k8-opt-exec-2B5CBBC6/bin/external/com_google_protobuf/protoc-2.params

Use --sandbox_debug to see verbose messages from the sandbox
clang: error: invalid linker name in argument '-fuse-ld=/usr/bin/ld.gold'
INFO: Elapsed time: 110.649s, Critical Path: 8.82s
INFO: 193 processes: 26 internal, 167 processwrapper-sandbox.
FAILED: Build did NOT complete successfully


# CC=clang CXX=clang bazel build //... --sandbox_debug
...
ERROR: /root/.cache/bazel/_bazel_root/e9e6a7354dba985d31e61d4d2962d0f8/external/com_google_protobuf/BUILD:354:10: Linking external/com_google_protobuf/protoc failed: (Exit 1): process-wrapper failed: error executing command 
  (cd /root/.cache/bazel/_bazel_root/e9e6a7354dba985d31e61d4d2962d0f8/sandbox/processwrapper-sandbox/172/execroot/curve && \
  exec env - \
    PATH=/root/.cache/bazelisk/downloads/bazelbuild/bazel-4.2.2-linux-x86_64/bin:/root/.vscode-server/bin/ee2b180d582a7f601fa6ecfdad8d9fd269ab1884/bin/remote-cli:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/local/go/bin:/root/go/bin \
    PWD=/proc/self/cwd \
    TMPDIR=/tmp \
  /root/.cache/bazel/_bazel_root/install/64841bf12de13c7518c7ada0994bafe2/process-wrapper '--timeout=0' '--kill_delay=15' /usr/bin/clang @bazel-out/k8-opt-exec-2B5CBBC6/bin/external/com_google_protobuf/protoc-2.params)
clang: error: invalid linker name in argument '-fuse-ld=/usr/bin/ld.gold'
INFO: Elapsed time: 0.507s, Critical Path: 0.06s
INFO: 18 processes: 18 internal.
FAILED: Build did NOT complete successfully

@wu-hanqing
Copy link
Contributor

Could someone please help review this pr?

Have you tried using Clang? the simplest way is CC=clang CXX=clang bazel build //...

I'm currently using docker for development, when I use clang I'll get some compile errors that seem caused by the environment, and I have no idea deal with it. So I only tried to compile directly.

What are the main mistakes? You can post it.

I created a new container and compile it with clang, then got:

# CC=clang CXX=clang bazel build //...
...
ERROR: /root/.cache/bazel/_bazel_root/e9e6a7354dba985d31e61d4d2962d0f8/external/com_google_protobuf/BUILD:354:10: Linking external/com_google_protobuf/protoc failed: (Exit 1): clang failed: error executing command /usr/bin/clang @bazel-out/k8-opt-exec-2B5CBBC6/bin/external/com_google_protobuf/protoc-2.params

Use --sandbox_debug to see verbose messages from the sandbox
clang: error: invalid linker name in argument '-fuse-ld=/usr/bin/ld.gold'
INFO: Elapsed time: 110.649s, Critical Path: 8.82s
INFO: 193 processes: 26 internal, 167 processwrapper-sandbox.
FAILED: Build did NOT complete successfully


# CC=clang CXX=clang bazel build //... --sandbox_debug
...
ERROR: /root/.cache/bazel/_bazel_root/e9e6a7354dba985d31e61d4d2962d0f8/external/com_google_protobuf/BUILD:354:10: Linking external/com_google_protobuf/protoc failed: (Exit 1): process-wrapper failed: error executing command 
  (cd /root/.cache/bazel/_bazel_root/e9e6a7354dba985d31e61d4d2962d0f8/sandbox/processwrapper-sandbox/172/execroot/curve && \
  exec env - \
    PATH=/root/.cache/bazelisk/downloads/bazelbuild/bazel-4.2.2-linux-x86_64/bin:/root/.vscode-server/bin/ee2b180d582a7f601fa6ecfdad8d9fd269ab1884/bin/remote-cli:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/local/go/bin:/root/go/bin \
    PWD=/proc/self/cwd \
    TMPDIR=/tmp \
  /root/.cache/bazel/_bazel_root/install/64841bf12de13c7518c7ada0994bafe2/process-wrapper '--timeout=0' '--kill_delay=15' /usr/bin/clang @bazel-out/k8-opt-exec-2B5CBBC6/bin/external/com_google_protobuf/protoc-2.params)
clang: error: invalid linker name in argument '-fuse-ld=/usr/bin/ld.gold'
INFO: Elapsed time: 0.507s, Critical Path: 0.06s
INFO: 18 processes: 18 internal.
FAILED: Build did NOT complete successfully

It seems this problem is related to clang-3.8
https://stackoverflow.com/questions/49997768/clang-3-8-error-invalid-linker-name-in-argument-fuse-ld-gold-2-25

I have tried clang-11 in development docker, it works, you can install clang-11 and retry.

apt install clang-11
CC=clang-11 CXX=clang++-11 bazel build //...

and, @Cyber-SiKu will update the default clang from 3.8 to 11 in soon

@MizukiCry
Copy link
Contributor Author

Could someone please help review this pr?

Have you tried using Clang? the simplest way is CC=clang CXX=clang bazel build //...

I'm currently using docker for development, when I use clang I'll get some compile errors that seem caused by the environment, and I have no idea deal with it. So I only tried to compile directly.

What are the main mistakes? You can post it.

I created a new container and compile it with clang, then got:

# CC=clang CXX=clang bazel build //...
...
ERROR: /root/.cache/bazel/_bazel_root/e9e6a7354dba985d31e61d4d2962d0f8/external/com_google_protobuf/BUILD:354:10: Linking external/com_google_protobuf/protoc failed: (Exit 1): clang failed: error executing command /usr/bin/clang @bazel-out/k8-opt-exec-2B5CBBC6/bin/external/com_google_protobuf/protoc-2.params

Use --sandbox_debug to see verbose messages from the sandbox
clang: error: invalid linker name in argument '-fuse-ld=/usr/bin/ld.gold'
INFO: Elapsed time: 110.649s, Critical Path: 8.82s
INFO: 193 processes: 26 internal, 167 processwrapper-sandbox.
FAILED: Build did NOT complete successfully


# CC=clang CXX=clang bazel build //... --sandbox_debug
...
ERROR: /root/.cache/bazel/_bazel_root/e9e6a7354dba985d31e61d4d2962d0f8/external/com_google_protobuf/BUILD:354:10: Linking external/com_google_protobuf/protoc failed: (Exit 1): process-wrapper failed: error executing command 
  (cd /root/.cache/bazel/_bazel_root/e9e6a7354dba985d31e61d4d2962d0f8/sandbox/processwrapper-sandbox/172/execroot/curve && \
  exec env - \
    PATH=/root/.cache/bazelisk/downloads/bazelbuild/bazel-4.2.2-linux-x86_64/bin:/root/.vscode-server/bin/ee2b180d582a7f601fa6ecfdad8d9fd269ab1884/bin/remote-cli:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/local/go/bin:/root/go/bin \
    PWD=/proc/self/cwd \
    TMPDIR=/tmp \
  /root/.cache/bazel/_bazel_root/install/64841bf12de13c7518c7ada0994bafe2/process-wrapper '--timeout=0' '--kill_delay=15' /usr/bin/clang @bazel-out/k8-opt-exec-2B5CBBC6/bin/external/com_google_protobuf/protoc-2.params)
clang: error: invalid linker name in argument '-fuse-ld=/usr/bin/ld.gold'
INFO: Elapsed time: 0.507s, Critical Path: 0.06s
INFO: 18 processes: 18 internal.
FAILED: Build did NOT complete successfully

It seems this problem is related to clang-3.8 https://stackoverflow.com/questions/49997768/clang-3-8-error-invalid-linker-name-in-argument-fuse-ld-gold-2-25

I have tried clang-11 in development docker, it works, you can install clang-11 and retry.

apt install clang-11
CC=clang-11 CXX=clang++-11 bazel build //...

and, @Cyber-SiKu will update the default clang from 3.8 to 11 in soon

OK, I tried it and it compiled successfully, but there comes many new warnings. If we have to fix them too, I think it's better to create another PR.

@wu-hanqing
Copy link
Contributor

OK, I tried it and it compiled successfully, but there comes many new warnings. If we have to fix them too, I think it's better to create another PR.

I think so, it's better to fix it in another PR, I will look through this PR ASAP.

@Cyber-SiKu
Copy link
Contributor

Could someone please help review this pr?

Have you tried using Clang? the simplest way is CC=clang CXX=clang bazel build //...

I'm currently using docker for development, when I use clang I'll get some compile errors that seem caused by the environment, and I have no idea deal with it. So I only tried to compile directly.

What are the main mistakes? You can post it.

I created a new container and compile it with clang, then got:

# CC=clang CXX=clang bazel build //...
...
ERROR: /root/.cache/bazel/_bazel_root/e9e6a7354dba985d31e61d4d2962d0f8/external/com_google_protobuf/BUILD:354:10: Linking external/com_google_protobuf/protoc failed: (Exit 1): clang failed: error executing command /usr/bin/clang @bazel-out/k8-opt-exec-2B5CBBC6/bin/external/com_google_protobuf/protoc-2.params

Use --sandbox_debug to see verbose messages from the sandbox
clang: error: invalid linker name in argument '-fuse-ld=/usr/bin/ld.gold'
INFO: Elapsed time: 110.649s, Critical Path: 8.82s
INFO: 193 processes: 26 internal, 167 processwrapper-sandbox.
FAILED: Build did NOT complete successfully


# CC=clang CXX=clang bazel build //... --sandbox_debug
...
ERROR: /root/.cache/bazel/_bazel_root/e9e6a7354dba985d31e61d4d2962d0f8/external/com_google_protobuf/BUILD:354:10: Linking external/com_google_protobuf/protoc failed: (Exit 1): process-wrapper failed: error executing command 
  (cd /root/.cache/bazel/_bazel_root/e9e6a7354dba985d31e61d4d2962d0f8/sandbox/processwrapper-sandbox/172/execroot/curve && \
  exec env - \
    PATH=/root/.cache/bazelisk/downloads/bazelbuild/bazel-4.2.2-linux-x86_64/bin:/root/.vscode-server/bin/ee2b180d582a7f601fa6ecfdad8d9fd269ab1884/bin/remote-cli:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/local/go/bin:/root/go/bin \
    PWD=/proc/self/cwd \
    TMPDIR=/tmp \
  /root/.cache/bazel/_bazel_root/install/64841bf12de13c7518c7ada0994bafe2/process-wrapper '--timeout=0' '--kill_delay=15' /usr/bin/clang @bazel-out/k8-opt-exec-2B5CBBC6/bin/external/com_google_protobuf/protoc-2.params)
clang: error: invalid linker name in argument '-fuse-ld=/usr/bin/ld.gold'
INFO: Elapsed time: 0.507s, Critical Path: 0.06s
INFO: 18 processes: 18 internal.
FAILED: Build did NOT complete successfully

It seems this problem is related to clang-3.8 https://stackoverflow.com/questions/49997768/clang-3-8-error-invalid-linker-name-in-argument-fuse-ld-gold-2-25
I have tried clang-11 in development docker, it works, you can install clang-11 and retry.

apt install clang-11
CC=clang-11 CXX=clang++-11 bazel build //...

and, @Cyber-SiKu will update the default clang from 3.8 to 11 in soon

OK, I tried it and it compiled successfully, but there comes many new warnings. If we have to fix them too, I think it's better to create another PR.

done

Copy link
Contributor

@wu-hanqing wu-hanqing left a comment

Choose a reason for hiding this comment

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

Please don't format unrelated code in future PR as it will add additional burden to code review.

curvefs/src/client/inode_wrapper.h Outdated Show resolved Hide resolved
curvefs/src/client/inode_wrapper.h Outdated Show resolved Hide resolved
curvefs/src/client/main.cpp Outdated Show resolved Hide resolved
curvefs/src/client/rpcclient/mds_client.cpp Show resolved Hide resolved
curvefs/src/client/rpcclient/metacache.cpp Outdated Show resolved Hide resolved
src/mds/schedule/leaderScheduler.cpp Outdated Show resolved Hide resolved
src/mds/topology/topology_storge_etcd.cpp Outdated Show resolved Hide resolved
src/snapshotcloneserver/snapshotclone_service.cpp Outdated Show resolved Hide resolved
src/tools/copyset_tool.cpp Outdated Show resolved Hide resolved
@MizukiCry
Copy link
Contributor Author

Please don't format unrelated code in future PR as it will add additional burden to code review.

Sorry for troubling you. I didn't notice that when I format the whole file. I'll avoid that next time.

@MizukiCry MizukiCry force-pushed the warning-clean-up branch 2 times, most recently from f2ff073 to d2f5e7f Compare April 23, 2023 13:53
@MizukiCry
Copy link
Contributor Author

cicheck

@wu-hanqing
Copy link
Contributor

cicheck

@MizukiCry
Copy link
Contributor Author

cicheck

@MizukiCry
Copy link
Contributor Author

cicheck

@MizukiCry
Copy link
Contributor Author

cicheck

Copy link
Contributor

@wu-hanqing wu-hanqing left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -88,7 +88,7 @@ std::string DiskCacheBase::GetCacheIoFullDir() {
int DiskCacheBase::CreateDir(const std::string dir) {
size_t p = dir.find_last_of('/');
std::string dirPath = dir;
if (p != -1) {
if (p != -1ULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (p != -1ULL) {
if (p != std::string::npos) {

You can fix this in the next PR if you want.

And we should finish this PR quickly, otherwise, when someone else merges PRs, they may introduce new warnings. 💢

@MizukiCry
Copy link
Contributor Author

@ilixiaocui This PR is finished. Please help review it:)

@ilixiaocui
Copy link
Contributor

LGTM!

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

Successfully merging this pull request may close these issues.

7 participants