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

[DWARFLinkerParallel] Fix member initialization order #81179

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Feb 8, 2024

DWARFLinkerImpl::DWARFLinkerImpl initializes
DebugStrStrings/DebugLineStrStrings/CommonSections using GlobalData
but GlobalData is initialized after the three members.
Move GlobalData before.

Fix #81110

Created using spr 1.3.4
@MaskRay MaskRay changed the title [DWARFLinkerParallel] Fix member initializer order [DWARFLinkerParallel] Fix member initialization order Feb 8, 2024
@MaskRay MaskRay merged commit 7c9c498 into main Feb 8, 2024
4 of 5 checks passed
@MaskRay MaskRay deleted the users/MaskRay/spr/dwarflinkerparallel-fix-member-initializer-order branch February 8, 2024 21:43
@dwblaikie
Copy link
Collaborator

I was concerned there might be missing test coverage here - but looks like the issue is benign (apart from the GCC warning itself) as the member is only stored as a reference and not used until after it's constructed.

Looking at Clang and GCC's behavior for the warning:

https://godbolt.org/z/G7ndsTv5c

It looks like GCC might have some quirks (it only warns about this case (escaping a reference to a member before the member is initialized) by the looks of it, if the ctor has only 1 parameter?).

Perhaps we should disable GCC's warning and rely on Clang's more conservative interpretation?

(though I don't actually object to this particular change - I think it's clearer/less subtle/etc - but in terms of what's worth filing bugs, etc about)

@MaskRay
Copy link
Member Author

MaskRay commented Feb 19, 2024

I was concerned there might be missing test coverage here - but looks like the issue is benign (apart from the GCC warning itself) as the member is only stored as a reference and not used until after it's constructed.

Looking at Clang and GCC's behavior for the warning:

godbolt.org/z/G7ndsTv5c

It looks like GCC might have some quirks (it only warns about this case (escaping a reference to a member before the member is initialized) by the looks of it, if the ctor has only 1 parameter?).

Perhaps we should disable GCC's warning and rely on Clang's more conservative interpretation?

(though I don't actually object to this particular change - I think it's clearer/less subtle/etc - but in terms of what's worth filing bugs, etc about)

Interesting. I did not realize that there's a clang-vs-GCC difference and the Clang behavior suppressing the diagnostic for a reference is intentional (seems around 2014).
(It seems that Clang's conservative behavior misses certain cases, e.g.

: A(IntWrapper(A)), // Due to a conservative implementation, we do not report warnings inside function/ctor calls even though it is possible to do so.
)

Suppressing -Wuninitialized for GCC sounds good.

Filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113987 ("Binding a reference to an uninitialized data member should not cause -Wuninitialized")

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.

llvm/trunk/llvm/lib/DWARFLinker/Parallel/DWARFLinkerImpl.cpp: 3 * used uninitialised ?
4 participants