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

[lld][NFC] Silence -Wuninitialized GCC warnings. #75183

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

cjacek
Copy link
Contributor

@cjacek cjacek commented Dec 12, 2023

A workaround for warnings reported in #69101. Use of those variables is guarded by lastType, so I don't think that they are actually used uninitialized and I don't see the warning on GCC 12.3.

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 12, 2023

@llvm/pr-subscribers-lld
@llvm/pr-subscribers-platform-windows

@llvm/pr-subscribers-lld-coff

Author: Jacek Caban (cjacek)

Changes

A workaround for warnings reported in #69101. Use of those variables is guarded by lastType, so I don't think that they are actually used uninitialized and I don't see the warning on GCC 12.3.


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

1 Files Affected:

  • (modified) lld/COFF/Writer.cpp (+1-1)
diff --git a/lld/COFF/Writer.cpp b/lld/COFF/Writer.cpp
index 7b1ff8071e2e3..89e0f5b2df744 100644
--- a/lld/COFF/Writer.cpp
+++ b/lld/COFF/Writer.cpp
@@ -560,7 +560,7 @@ void Writer::createECCodeMap() {
   codeMap.clear();
 
   std::optional<chpe_range_type> lastType;
-  Chunk *first, *last;
+  Chunk *first = nullptr, *last = nullptr;
 
   auto closeRange = [&]() {
     if (lastType) {

Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

FTR, I did run into these warnings with GCC 11.

@cjacek cjacek merged commit ccec22b into llvm:main Dec 12, 2023
6 of 7 checks passed
@dwblaikie
Copy link
Collaborator

I believe clang does a better job with sometimes-ininitialized warnings, perhaps we should just disable GCC's overly conservative warning, rather than adding unnecessary initializers? (that would hinder Clang's better warning and sanitizer detections)

@MaskRay
Copy link
Member

MaskRay commented Dec 18, 2023

I believe clang does a better job with sometimes-ininitialized warnings, perhaps we should just disable GCC's overly conservative warning, rather than adding unnecessary initializers? (that would hinder Clang's better warning and sanitizer detections)

I agree. I have seen several instances that GCC -Wuninitialized is annoying than useful.

@mstorsjo
Copy link
Member

I believe clang does a better job with sometimes-ininitialized warnings, perhaps we should just disable GCC's overly conservative warning, rather than adding unnecessary initializers? (that would hinder Clang's better warning and sanitizer detections)

I agree. I have seen several instances that GCC -Wuninitialized is annoying than useful.

In this case, it seems like the issue only exists in older GCC versions though, but I'm not sure if we'd like to do such a change depending on GCC version either. So perhaps it would indeed be best to skip such warnings altogether on GCC.

@dwblaikie
Copy link
Collaborator

I believe clang does a better job with sometimes-ininitialized warnings, perhaps we should just disable GCC's overly conservative warning, rather than adding unnecessary initializers? (that would hinder Clang's better warning and sanitizer detections)

I agree. I have seen several instances that GCC -Wuninitialized is annoying than useful.

In this case, it seems like the issue only exists in older GCC versions though, but I'm not sure if we'd like to do such a change depending on GCC version either. So perhaps it would indeed be best to skip such warnings altogether on GCC.

We could disable the warning only on the GCC versions where it has a false positive - either by checking the version, or by writing a small snippet that tests the false positive case, and disabling if that snippet produces a warning. (I think we use this kind of code-based feature detection for some warning enablement/disablement - or have in the past - not sure exactly where it lives)

@cjacek
Copy link
Contributor Author

cjacek commented Dec 22, 2023

I found that we already disable -Wmaybe-uninitialized. I created #76251 which disables all -Wuninitialized for GCCs older than 11. We could potentially try to utilize CHECK_C_SOURCE_COMPILES, but for cases like this, it's tricky to find reliable and simple enough tests case. I ended up using a compiler version check, but we could easily change it to be unconditional if that's preferred (that would mean that for some developers warnings would be unnoticed before pushing, through).

If/when #76251 lands, I will revert this PR.

cjacek added a commit that referenced this pull request Dec 26, 2023
…76251)

As discussed in #75183, avoids dealing with GCC false positives.
cjacek added a commit to cjacek/llvm-project that referenced this pull request Dec 26, 2023
cjacek added a commit that referenced this pull request Dec 26, 2023
This reverts commit ccec22b (#75183).
It's no longer needed with #76251.
@dwblaikie
Copy link
Collaborator

Could we revert this now that the warning's been disabled?

@cjacek
Copy link
Contributor Author

cjacek commented Jan 4, 2024

It's already reverted in #76398.

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.

None yet

5 participants