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: get llvm type of global val #128820

Merged
merged 1 commit into from
Sep 5, 2024
Merged

fix: get llvm type of global val #128820

merged 1 commit into from
Sep 5, 2024

Conversation

LYF1999
Copy link
Contributor

@LYF1999 LYF1999 commented Aug 8, 2024

using LLVMTypeOf on a global var always return ptr. so create a new function to access the value type of a global

@rustbot
Copy link
Collaborator

rustbot commented Aug 8, 2024

r? @cuviper

rustbot has assigned @cuviper.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Aug 8, 2024
@rust-log-analyzer

This comment has been minimized.

@jieyouxu jieyouxu added the A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. label Aug 8, 2024
@jieyouxu
Copy link
Member

jieyouxu commented Aug 8, 2024

cc @rust-lang/wg-llvm

@DianQK
Copy link
Member

DianQK commented Aug 8, 2024

Could you add a test to show your change?

@LYF1999
Copy link
Contributor Author

LYF1999 commented Aug 9, 2024

Could you add a test to show your change?

I don't know how to make a test to show this change. this bug does not cause any compilation issues and they produce correct llvm ir .
if the ty not equal, we will create a new global for the initializer and replace old one.
judging from the intent of the code here, we should compare the global value ty rather than global self ty(always a ptr)

@LYF1999 LYF1999 requested a review from nikic August 9, 2024 03:11
@@ -390,7 +390,7 @@ impl<'ll> CodegenCx<'ll, '_> {
let val_llty = self.val_ty(v);

let g = self.get_static_inner(def_id, val_llty);
let llty = self.val_ty(g);
let llty = llvm::LLVMGlobalGetValueType(g);
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 please also adjust the self.val_ty(g) != self.type_ptr() check above? That one is also supposed to read LLVMGlobalGetValueType(g) != llty I believe.

Copy link
Contributor Author

@LYF1999 LYF1999 Aug 14, 2024

Choose a reason for hiding this comment

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

hm... i found the initializer type is always a packed struct(val_llty) at now. which can never be equal to the llty.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

why do we use cx.const_struct(&llvals, true) to create a packed const struct at const_alloc_to_llvm. can we create a normal struct for static initializer

Copy link
Contributor

Choose a reason for hiding this comment

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

hm... i found the initializer type is always a packed struct(val_ty) at now. which can never be equal to the llty.

But isn't this also the type we pass as llty? It's the type of the initializer v above, which should also be a packed struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the llty is the type of old global var, which is set at predefine_static. and the val_llty is the type of initializer v.
the llty won't be a packed struct

@cuviper
Copy link
Member

cuviper commented Aug 22, 2024

r? nikic
(to make sure your request is resolved)

@rustbot rustbot assigned nikic and unassigned cuviper Aug 22, 2024
@nikic
Copy link
Contributor

nikic commented Sep 5, 2024

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 5, 2024

📌 Commit 27f92b6 has been approved by nikic

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 5, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Sep 5, 2024
fix: get llvm type of global val

using `LLVMTypeOf` on a global var always return ptr. so create a new function to access the value type of a global
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#128820 (fix: get llvm type of global val)
 - rust-lang#129028 (`impl_trait_overcaptures`: Don't worry about uncaptured contravariant lifetimes if they outlive a captured lifetime)
 - rust-lang#129471 ([rustdoc] Sort impl associated items by kinds and then by appearance)
 - rust-lang#129706 (Rename dump of coroutine by-move-body to be more consistent, fix ICE in dump_mir)
 - rust-lang#129720 (Simplify DestProp memory management)
 - rust-lang#129796 (Unify scraped examples with other code examples)
 - rust-lang#129938 (Elaborate on deriving vs implementing `Copy`)
 - rust-lang#129973 (run_make_support: rename `Command::stdin` to `stdin_buf` and add `std{in,out,err}` config helpers)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#128820 (fix: get llvm type of global val)
 - rust-lang#129028 (`impl_trait_overcaptures`: Don't worry about uncaptured contravariant lifetimes if they outlive a captured lifetime)
 - rust-lang#129471 ([rustdoc] Sort impl associated items by kinds and then by appearance)
 - rust-lang#129706 (Rename dump of coroutine by-move-body to be more consistent, fix ICE in dump_mir)
 - rust-lang#129720 (Simplify DestProp memory management)
 - rust-lang#129796 (Unify scraped examples with other code examples)
 - rust-lang#129938 (Elaborate on deriving vs implementing `Copy`)
 - rust-lang#129973 (run_make_support: rename `Command::stdin` to `stdin_buf` and add `std{in,out,err}` config helpers)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit b89ee99 into rust-lang:master Sep 5, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 5, 2024
Rollup merge of rust-lang#128820 - LYF1999:yf/dev, r=nikic

fix: get llvm type of global val

using `LLVMTypeOf` on a global var always return ptr. so create a new function to access the value type of a global
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-LLVM Area: Code generation parts specific to LLVM. Both correctness bugs and optimization-related issues. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants