codegen_llvm: set `DW_AT_accessibility` #115165

davidtwco · 2023-08-24T09:35:25Z

Fixes #9228.
Based on #74778.

Sets the accessibility of types and fields in DWARF using DW_AT_accessibility attribute.

DW_AT_accessibility (public/protected/private) isn't exactly right for Rust, but neither is DW_AT_visibility (local/exported/qualified), and there's no way to set DW_AT_visbility in LLVM's API. Debuggers will special-case the handling of these per-language anyway.

r? @wesleywiser (visited in wg-debugging triage)

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs

wesleywiser · 2023-09-26T19:31:11Z

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs

+ type_did: DefId,
+) -> DIFlags {
+ let parent_did = cx.tcx.parent(type_did);
+ let visibility = cx.tcx.visibility(did);


I was going to comment that it might make more sense to calculate the visibility from the perspective of the crate root (or, rather, as if you were using the crate as a dependency?) but I don't see why that necessarily be better and the current implementation is simpler and matches the visibility of the declaration site (even if the effective visibility ends up being more hidden due to the visibility/exports of parent modules).

Just noting that is another possible way to implement this but I don't see a strong reason to do that at this time.

wesleywiser · 2023-09-26T19:31:26Z

Thanks @davidtwco!

@bors r+

bors · 2023-09-26T19:31:28Z

📌 Commit 1f83e77 has been approved by wesleywiser

It is now in the queue for this repository.

bors · 2023-09-26T20:55:01Z

⌛ Testing commit 1f83e77 with merge fe9519c...

…ember-visibility, r=wesleywiser codegen_llvm: set `DW_AT_accessibility` Fixes rust-lang#9228. Based on rust-lang#74778. Sets the accessibility of types and fields in DWARF using `DW_AT_accessibility` attribute. `DW_AT_accessibility` (public/protected/private) isn't exactly right for Rust, but neither is `DW_AT_visibility` (local/exported/qualified), and there's no way to set `DW_AT_visbility` in LLVM's API. Debuggers will special-case the handling of these per-language anyway. r? `@wesleywiser` (visited in wg-debugging triage)

bors · 2023-09-26T21:31:37Z

💔 Test failed - checks-actions

davidtwco · 2023-10-03T12:13:59Z

I've split the test up into different files for each type - as far as I can tell the test fails in CI because of the order of items in the LLVM IR.

wesleywiser · 2023-10-25T13:52:23Z

@bors r+

bors · 2023-10-25T13:52:25Z

📌 Commit 02682de has been approved by wesleywiser

It is now in the queue for this repository.

bors · 2023-10-25T21:23:09Z

⌛ Testing commit 02682de with merge 4ab4817...

…ember-visibility, r=wesleywiser codegen_llvm: set `DW_AT_accessibility` Fixes rust-lang#9228. Based on rust-lang#74778. Sets the accessibility of types and fields in DWARF using `DW_AT_accessibility` attribute. `DW_AT_accessibility` (public/protected/private) isn't exactly right for Rust, but neither is `DW_AT_visibility` (local/exported/qualified), and there's no way to set `DW_AT_visbility` in LLVM's API. Debuggers will special-case the handling of these per-language anyway. r? `@wesleywiser` (visited in wg-debugging triage)

bors · 2023-10-25T22:16:04Z

💔 Test failed - checks-actions

davidtwco · 2023-12-15T11:48:14Z

Rebased and fixed tidy errors, will r+ once CI is passing.

davidtwco · 2023-12-15T13:19:03Z

@bors r=wesleywiser

bors · 2023-12-15T13:19:06Z

📌 Commit ce29051 has been approved by wesleywiser

It is now in the queue for this repository.

bors · 2023-12-15T14:49:56Z

⌛ Testing commit ce29051 with merge 47a262c...

…ember-visibility, r=wesleywiser codegen_llvm: set `DW_AT_accessibility` Fixes rust-lang#9228. Based on rust-lang#74778. Sets the accessibility of types and fields in DWARF using `DW_AT_accessibility` attribute. `DW_AT_accessibility` (public/protected/private) isn't exactly right for Rust, but neither is `DW_AT_visibility` (local/exported/qualified), and there's no way to set `DW_AT_visbility` in LLVM's API. Debuggers will special-case the handling of these per-language anyway. r? `@wesleywiser` (visited in wg-debugging triage)

bors · 2023-12-15T14:56:37Z

💔 Test failed - checks-actions

rust-log-analyzer · 2023-12-15T15:01:06Z

The job dist-aarch64-apple failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

SCRIPT=./x.py dist bootstrap --include-default-paths --host=aarch64-apple-darwin --target=aarch64-apple-darwin
SELECT_XCODE=/Applications/Xcode_13.4.1.app
SHELL=/bin/bash
SHLVL=3
SSH_AUTH_SOCK=/private/tmp/com.apple.launchd.WcUT1Lj5lV/Listeners
STATS_EXT=true
STATS_EXTP=https://provjobdsettingscdn.blob.core.windows.net/settings/provjobdsettings-0.5.154/provjobd.data
STATS_RDCL=true
STATS_TIS=mining
---
sccache: warning: The server looks like it shut down unexpectedly, compiling locally instead
sccache: warning: The server looks like it shut down unexpectedly, compiling locally instead
[ 36%] Building CXX object lib/Target/RISCV/MCTargetDesc/CMakeFiles/LLVMRISCVDesc.dir/RISCVBaseInfo.cpp.o
sccache: warning: The server looks like it shut down unexpectedly, compiling locally instead
sccache: error: failed to execute compile
sccache: caused by: Failed to send data to or receive data from server
sccache: caused by: Failed to read response header
sccache: caused by: Connection reset by peer (os error 54)
make[2]: *** [lib/Target/NVPTX/MCTargetDesc/CMakeFiles/LLVMNVPTXDesc.dir/NVPTXTargetStreamer.cpp.o] Error 2
make[1]: *** [lib/Target/NVPTX/MCTargetDesc/CMakeFiles/LLVMNVPTXDesc.dir/all] Error 2
make[1]: *** Waiting for unfinished jobs....
[ 36%] Building CXX object lib/Target/RISCV/MCTargetDesc/CMakeFiles/LLVMRISCVDesc.dir/RISCVInstPrinter.cpp.o
[ 36%] Building CXX object lib/Target/Sparc/MCTargetDesc/CMakeFiles/LLVMSparcDesc.dir/SparcELFObjectWriter.cpp.o
[ 36%] Building CXX object lib/Target/Sparc/MCTargetDesc/CMakeFiles/LLVMSparcDesc.dir/SparcInstPrinter.cpp.o
[ 36%] Building CXX object lib/Target/RISCV/MCTargetDesc/CMakeFiles/LLVMRISCVDesc.dir/RISCVMCAsmInfo.cpp.o

wesleywiser · 2023-12-15T16:23:58Z

2023-12-15T14:55:36.8253420Z [ 36%] Building CXX object lib/Target/NVPTX/MCTargetDesc/CMakeFiles/LLVMNVPTXDesc.dir/NVPTXTargetStreamer.cpp.o
2023-12-15T14:55:36.8512930Z sccache: warning: The server looks like it shut down unexpectedly, compiling locally instead
2023-12-15T14:55:36.8629180Z sccache: warning: The server looks like it shut down unexpectedly, compiling locally instead
2023-12-15T14:55:36.8754600Z sccache: warning: The server looks like it shut down unexpectedly, compiling locally instead
2023-12-15T14:55:36.8854030Z sccache: warning: The server looks like it shut down unexpectedly, compiling locally instead
2023-12-15T14:55:36.8854890Z [ 36%] Building CXX object lib/Target/RISCV/MCTargetDesc/CMakeFiles/LLVMRISCVDesc.dir/RISCVBaseInfo.cpp.o
2023-12-15T14:55:36.8956170Z sccache: warning: The server looks like it shut down unexpectedly, compiling locally instead
2023-12-15T14:55:36.9055780Z sccache: error: failed to execute compile
2023-12-15T14:55:36.9156310Z sccache: caused by: Failed to send data to or receive data from server
2023-12-15T14:55:36.9256960Z sccache: caused by: Failed to read response header
2023-12-15T14:55:36.9357660Z sccache: caused by: Connection reset by peer (os error 54)
2023-12-15T14:55:36.9460890Z make[2]: *** [lib/Target/NVPTX/MCTargetDesc/CMakeFiles/LLVMNVPTXDesc.dir/NVPTXTargetStreamer.cpp.o] Error 2
2023-12-15T14:55:36.9568450Z make[1]: *** [lib/Target/NVPTX/MCTargetDesc/CMakeFiles/LLVMNVPTXDesc.dir/all] Error 2
2023-12-15T14:55:36.9676090Z make[1]: *** Waiting for unfinished jobs....

@bors retry

bors · 2023-12-15T17:08:59Z

⌛ Testing commit ce29051 with merge 6af6885...

…ember-visibility, r=wesleywiser codegen_llvm: set `DW_AT_accessibility` Fixes rust-lang#9228. Based on rust-lang#74778. Sets the accessibility of types and fields in DWARF using `DW_AT_accessibility` attribute. `DW_AT_accessibility` (public/protected/private) isn't exactly right for Rust, but neither is `DW_AT_visibility` (local/exported/qualified), and there's no way to set `DW_AT_visbility` in LLVM's API. Debuggers will special-case the handling of these per-language anyway. r? `@wesleywiser` (visited in wg-debugging triage)

bors · 2023-12-15T17:13:08Z

⌛ Testing commit ce29051 with merge 3f39cae...

rust-log-analyzer · 2023-12-15T17:18:19Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

bors · 2023-12-15T19:31:10Z

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 3f39cae to master...

rust-timer · 2023-12-15T21:59:48Z

Finished benchmarking commit (3f39cae): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.2%, 0.3%]	4
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.7%	[-0.7%, -0.6%]	4
Improvements ✅ (secondary)	-0.4%	[-0.7%, -0.2%]	22
All ❌✅ (primary)	-0.2%	[-0.7%, 0.3%]	8

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.9%	[0.1%, 1.6%]	2
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-1.9%	[-3.3%, -0.5%]	2
Improvements ✅ (secondary)	-1.7%	[-2.2%, -1.1%]	2
All ❌✅ (primary)	-0.5%	[-3.3%, 1.6%]	4

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-3.9%	[-3.9%, -3.9%]	1
Improvements ✅ (secondary)	-2.3%	[-2.6%, -2.1%]	5
All ❌✅ (primary)	-3.9%	[-3.9%, -3.9%]	1

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.3%	[0.0%, 0.8%]	61
Regressions ❌ (secondary)	0.2%	[0.1%, 0.4%]	15
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.3%	[0.0%, 0.8%]	61

Bootstrap: 673.805s -> 671.537s (-0.34%)
Artifact size: 312.56 MiB -> 312.53 MiB (-0.01%)

lqd · 2023-12-16T12:42:19Z

The wins here are actually the end of the noise spike from the PR merged before this one returning to steady-state.

The 4 regressions look real though and make sense, with more debuginfo data (seen in the binary size increase), and that should come with more work e.g. in the linker (cc @Kobzol: maybe we should normalize/show absolute values in the frontend/backend/linker perf.rlo sections for these cases where we know the total and ratio have changed, but can't compare the sections themselves as they're relative. But maybe using wall-time, until we re-land the switch to storing icounts in measureme, and noise would prevent from seeing actual signal in these small changes there anyways).

Note that we can put a lesser emphasis on the 2 regressions on syn, the biggest binary size increase on serde_derive and so on, they shouldn't be impacted in real-world dependency trees as proc-macros are built without debuginfo by cargo.

The regressions are pretty small, so probably not worth looking into much further imho.

pnkfelix · 2023-12-19T05:15:13Z

I agree with @lqd's analysis and am marking as triaged

@rustbot label: +perf-regression-triaged

rustbot assigned wesleywiser Aug 24, 2023

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 24, 2023

bjorn3 reviewed Sep 5, 2023

View reviewed changes

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs Outdated Show resolved Hide resolved

davidtwco force-pushed the issue-9228-describe-item-member-visibility branch from 376f738 to 1f83e77 Compare September 6, 2023 15:43

wesleywiser approved these changes Sep 26, 2023

View reviewed changes

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 26, 2023

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Sep 26, 2023

davidtwco force-pushed the issue-9228-describe-item-member-visibility branch from 1f83e77 to 02682de Compare October 3, 2023 12:13

wesleywiser approved these changes Oct 25, 2023

View reviewed changes

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 Oct 25, 2023

This comment has been minimized.

Sign in to view

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 25, 2023

davidtwco force-pushed the issue-9228-describe-item-member-visibility branch from 02682de to 1b857fb Compare November 6, 2023 02:48

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 Dec 15, 2023

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 15, 2023

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 Dec 15, 2023

bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 15, 2023

bors merged commit 3f39cae into rust-lang:master Dec 15, 2023
12 checks passed

rustbot added this to the 1.76.0 milestone Dec 15, 2023

rustbot added the perf-regression Performance regression. label Dec 15, 2023

lqd mentioned this pull request Dec 16, 2023

Rollup of 3 pull requests #118982

Merged

rustbot added the perf-regression-triaged The performance regression has been triaged. label Dec 19, 2023

davidtwco deleted the issue-9228-describe-item-member-visibility branch January 3, 2024 11:34

noppej mentioned this pull request Feb 5, 2024

Debugger: Cannot resolve variables with DW_AT_accessibility attributes probe-rs/probe-rs#2140

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

codegen_llvm: set `DW_AT_accessibility` #115165

codegen_llvm: set `DW_AT_accessibility` #115165

davidtwco commented Aug 24, 2023

wesleywiser Sep 26, 2023

wesleywiser commented Sep 26, 2023

bors commented Sep 26, 2023

bors commented Sep 26, 2023

This comment has been minimized.

bors commented Sep 26, 2023

davidtwco commented Oct 3, 2023

wesleywiser commented Oct 25, 2023

bors commented Oct 25, 2023

bors commented Oct 25, 2023

This comment has been minimized.

bors commented Oct 25, 2023

davidtwco commented Dec 15, 2023

davidtwco commented Dec 15, 2023

bors commented Dec 15, 2023

bors commented Dec 15, 2023

bors commented Dec 15, 2023

rust-log-analyzer commented Dec 15, 2023

wesleywiser commented Dec 15, 2023

bors commented Dec 15, 2023

bors commented Dec 15, 2023

rust-log-analyzer commented Dec 15, 2023

bors commented Dec 15, 2023

rust-timer commented Dec 15, 2023

lqd commented Dec 16, 2023

pnkfelix commented Dec 19, 2023

codegen_llvm: set DW_AT_accessibility #115165

codegen_llvm: set DW_AT_accessibility #115165

Conversation

davidtwco commented Aug 24, 2023

wesleywiser Sep 26, 2023

Choose a reason for hiding this comment

wesleywiser commented Sep 26, 2023

bors commented Sep 26, 2023

bors commented Sep 26, 2023

This comment has been minimized.

bors commented Sep 26, 2023

davidtwco commented Oct 3, 2023

wesleywiser commented Oct 25, 2023

bors commented Oct 25, 2023

bors commented Oct 25, 2023

This comment has been minimized.

bors commented Oct 25, 2023

davidtwco commented Dec 15, 2023

davidtwco commented Dec 15, 2023

bors commented Dec 15, 2023

bors commented Dec 15, 2023

bors commented Dec 15, 2023

rust-log-analyzer commented Dec 15, 2023

wesleywiser commented Dec 15, 2023

bors commented Dec 15, 2023

bors commented Dec 15, 2023

rust-log-analyzer commented Dec 15, 2023

bors commented Dec 15, 2023

rust-timer commented Dec 15, 2023

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

Binary size

lqd commented Dec 16, 2023

pnkfelix commented Dec 19, 2023

codegen_llvm: set `DW_AT_accessibility` #115165

codegen_llvm: set `DW_AT_accessibility` #115165