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

[clang-repl] Emit const variables only once #65257

Merged
merged 1 commit into from
Oct 3, 2023
Merged

[clang-repl] Emit const variables only once #65257

merged 1 commit into from
Oct 3, 2023

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Sep 4, 2023

Disable internal linkage for const variables if IncrementalExtensions are enabled. Otherwise the variables are emitted multiple times, with multiple constructions at unique memory locations, during every PTU.

@hahnjo hahnjo added the clang:frontend Language frontend issues, e.g. anything involving "Sema" label Sep 4, 2023
@hahnjo hahnjo requested review from iains and vgvassilev September 4, 2023 12:23
@hahnjo hahnjo requested a review from a team as a code owner September 4, 2023 12:23
@vgvassilev vgvassilev requested a review from rjmccall September 4, 2023 12:35
vgvassilev
vgvassilev approved these changes Sep 4, 2023
View reviewed changes
Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

That looks good to me. Can we backport that to the llvm17 release branch? I know at least one downstream project that jumps through hoops to support this.

@AaronBallman
Copy link
Collaborator

That looks good to me. Can we backport that to the llvm17 release branch? I know at least one downstream project that jumps through hoops to support this.

I'd say we should not backport to 17.0.0; we need to get the final rc out the door and this isn't fixing a critical bug or regression. However, it may be reasonable to consider for 17.0.1+

@hahnjo
[clang-repl] Emit const variables only once
7b52d2a 
Disable internal linkage for const variables if IncrementalExtensions
are enabled. Otherwise the variables are emitted multiple times, with
multiple constructions at unique memory locations, during every PTU.
@hahnjo
Copy link
Member Author

hahnjo commented Sep 6, 2023

The original patch worked for clang-repl but results in strong linkage which I found to cause problems with modules downstream in ROOT. Instead the latest push moves the special case one level higher to basicGVALinkageForVariable and returns GVA_DiscardableODR which fixes that problem as well.

@hahnjo hahnjo requested a review from vgvassilev September 6, 2023 11:15
@hahnjo
Copy link
Member Author

hahnjo commented Sep 18, 2023

Ping on the updated patch, which is now also passing downstream testing after I realized that we were missing a backport of commit e451d55, which explains why some constructors were wrongly ordered...

iains
iains reviewed Sep 28, 2023
View reviewed changes
Copy link
Contributor

@iains iains left a comment

Choose a reason for hiding this comment

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

(still getting used to the revised system)
I have no comments on this patch.

@hahnjo
Copy link
Member Author

hahnjo commented Sep 29, 2023

ping @vgvassilev

vgvassilev
vgvassilev reviewed Sep 29, 2023
View reviewed changes
clang/test/Interpreter/const.cpp
// RUN: cat %s | clang-repl -Xcc -O2 | FileCheck %s

extern "C" int printf(const char*, ...);

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a case for a file scope constant? We also should consider enabling that for C.

Copy link
Member Author

Choose a reason for hiding this comment

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

const A a(1); is a file-scope constant, no? We don't need it for C because the special case in LinkageComputer::getLVForNamespaceScopeDecl only applies to C++ (my understanding is that const variables always have an identity in C).

Copy link
Contributor

Choose a reason for hiding this comment

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

const A a(1); is a file-scope constant, no?

Yes, missed that.

vgvassilev
vgvassilev approved these changes Oct 3, 2023
View reviewed changes
Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

LGTM. Let's move forward here and rely on a further post-commit review if necessary.

clang/test/Interpreter/const.cpp
// RUN: cat %s | clang-repl -Xcc -O2 | FileCheck %s

extern "C" int printf(const char*, ...);

Copy link
Contributor

Choose a reason for hiding this comment

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

const A a(1); is a file-scope constant, no?

Yes, missed that.

@hahnjo hahnjo merged commit 05137ec into llvm:main Oct 3, 2023
@hahnjo hahnjo deleted the clang-repl-const branch October 3, 2023 09:58
@hahnjo hahnjo mentioned this pull request Oct 3, 2023
Open
@hahnjo
Copy link
Member Author

hahnjo commented Oct 3, 2023

Note that this currently doesn't seem to work on Windows: #68092

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iains iains iains left review comments

@vgvassilev vgvassilev vgvassilev approved these changes

@rjmccall rjmccall Awaiting requested review from rjmccall

Assignees
No one assigned
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema"
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@hahnjo @AaronBallman @iains @vgvassilev