NotificationsYou must be signed in to change notification settings
Fork14.1k
Star108k

Add Natvis visualiser and debuginfo tests for`f16`#127001

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

bors merged 1 commit intorust-lang:masterfrombeetrees:f16-debuginfo

Jul 9, 2024

Merged

Add Natvis visualiser and debuginfo tests for`f16`#127001

bors merged 1 commit intorust-lang:masterfrombeetrees:f16-debuginfo

Jul 9, 2024

Conversation

Copy link

Contributor

beetrees commentedJun 26, 2024

To renderf16s in debuggers on MSVC targets, this PR changes the compiler to outputf16s asstruct f16 { bits: u16 }, and includes a Natvis visualiser that manually converts thef16's bits to afloat which is can then be displayed by debuggers.gdb,lldb andcdb tests are also included forf16 .

f16/f128 MSVC debug info issue:#121837
Tracking issue:#116909

rustbot assignedestebank

Jun 26, 2024

Copy link

Collaborator

rustbot commentedJun 26, 2024

r?@estebank

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

User? to explicitly pick a reviewer

rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-compilerRelevant to the compiler team, which will review and decide on the PR/issue. labels

Jun 26, 2024

beetrees force-pushed thef16-debuginfo branch from197a29c to64d6157Compare

June 26, 2024 19:47

beetrees marked this pull request as ready for review

June 26, 2024 19:47

Copy link

ContributorAuthor

beetrees commentedJun 26, 2024

@rustbot label +F-f16_and_f128

rustbot added the F-f16_and_f128`#![feature(f16)]`, `#![feature(f128)]` label

Jun 26, 2024

Copy link

Contributor

tgross35 commentedJun 27, 2024

Cc@MaulingMonkey since you suggested what I copied to#121837

tgross35 reviewed

Jun 27, 2024

View reviewed changes

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs OutdatedShow resolvedHide resolved

beetrees force-pushed thef16-debuginfo branch from64d6157 to0a87558Compare

June 27, 2024 15:05

Copy link

Contributor

tgross35 commentedJun 27, 2024•
edited
Loading

@ChrisDenton do you handle the natvis stuff? I seem to remember there not being too many people who were able to review it (though this seems pretty understandable, the math part in intrinsic.natvis looks correct to me)

Copy link

Member

ChrisDenton commentedJun 27, 2024

No, sorry. While I have a little familiarity with the format, I wouldn't trust myself to review. Try@michaelwoerister maybe?

Copy link

Member

michaelwoerister commentedJun 27, 2024

r?@michaelwoerister
I'll take a look next week.

rustbot assignedmichaelwoerister and unassignedestebank

Jun 27, 2024

Copy link

Member

michaelwoerister commentedJun 27, 2024

C# seems to have something like f16, so maybe we are lucky:
https://devblogs.microsoft.com/dotnet/introducing-the-half-type/

michaelwoerister approved these changes

Jul 4, 2024

View reviewed changes

Copy link

Member

michaelwoerister left a comment

Choose a reason for hiding this comment

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

This looks great, thanks@beetrees!
I have only some minor comments.

tests/debuginfo/f16-natvis.rs

		@@ -0,0 +1,56 @@
		//@ compile-flags: -g

Copy link

Member

michaelwoeristerJul 4, 2024

Choose a reason for hiding this comment

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

Can you add//@ only-windows so that we ignore the test on other platforms?

Copy link

ContributorAuthor

beetreesJul 4, 2024

Choose a reason for hiding this comment

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

I've added//@ only-msvc ascpp_like_debuginfo is only used on MSVC targets.

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs

		}
		}

		fnbuild_cpp_f16_di_node<'ll,'tcx>(cx:&CodegenCx<'ll,'tcx>) ->DINodeCreationResult<'ll>{

Copy link

Member

michaelwoeristerJul 4, 2024

Choose a reason for hiding this comment

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

Please add a comment explaining why we do this.

Copy link

ContributorAuthor

beetreesJul 4, 2024

Choose a reason for hiding this comment

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

Added

beetrees force-pushed thef16-debuginfo branch from0a87558 tob701222Compare

July 4, 2024 15:58

Copy link

Member

michaelwoerister commentedJul 5, 2024

Thanks for the updates,@beetrees!

Excluding from rollups since debuginfo tests often fail on the first try in CI.
@bors r+ rollup=never

Copy link

Collaborator

bors commentedJul 5, 2024

📌 Commitb701222 has been approved bymichaelwoerister

It is now in thequeue for this repository.

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

Jul 5, 2024

Copy link

Collaborator

bors commentedJul 5, 2024

⌛ Testing commitb701222 with merge66fe6dd...

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jul 5, 2024

Auto merge ofrust-lang#127001- beetrees:f16-debuginfo, r=michaelwoe…

66fe6dd

…risterAdd Natvis visualiser and debuginfo tests for `f16`To render `f16`s in debuggers on MSVC targets, this PR changes the compiler to output `f16`s as `struct f16 { bits: u16 }`, and includes a Natvis visualiser that manually converts the `f16`'s bits to a `float` which is can then be displayed by debuggers. `gdb`, `lldb` and `cdb` tests are also included for `f16` .`f16`/`f128` MSVC debug info issue:rust-lang#121837Tracking issue:rust-lang#116909

This comment has been minimized.

Copy link

Collaborator

bors commentedJul 5, 2024

💔 Test failed -checks-actions

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

Jul 5, 2024

michaelwoerister reviewed

Jul 5, 2024

View reviewed changes

tests/debuginfo/basic-types-mut-globals.rs

		// gdb-check:$30 = 9.25

		#![allow(unused_variables)]
		#![feature(omit_gdb_pretty_printer_section)]

Copy link

Member

michaelwoeristerJul 5, 2024

Choose a reason for hiding this comment

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

Needs a#![feature(f16)].

If you're up for it you can try to remove the//@ ignore-gdb directive at the top, since the very similarbasic-types-globals.rs test seems to work just fine.

Copy link

ContributorAuthor

beetreesJul 5, 2024

Choose a reason for hiding this comment

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

I've added the missing#![feature(f16)]s, and removed the//@ ignore-gdb (the test passes locally).

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jul 8, 2024

Auto merge ofrust-lang#127001- beetrees:f16-debuginfo, r=michaelwoe…

f2bd5e4

…risterAdd Natvis visualiser and debuginfo tests for `f16`To render `f16`s in debuggers on MSVC targets, this PR changes the compiler to output `f16`s as `struct f16 { bits: u16 }`, and includes a Natvis visualiser that manually converts the `f16`'s bits to a `float` which is can then be displayed by debuggers. `gdb`, `lldb` and `cdb` tests are also included for `f16` .`f16`/`f128` MSVC debug info issue:rust-lang#121837Tracking issue:rust-lang#116909

This comment has been minimized.

Copy link

Collaborator

bors commentedJul 8, 2024

💔 Test failed -checks-actions

Jul 8, 2024

Copy link

Member

compiler-errors commentedJul 8, 2024

@bors retry

Jul 8, 2024

Copy link

Collaborator

bors commentedJul 9, 2024

⌛ Testing commit4252842 with merge585ffed...

bors added a commit to rust-lang-ci/rust that referenced this pull request

Jul 9, 2024

Auto merge ofrust-lang#127001- beetrees:f16-debuginfo, r=michaelwoe…

585ffed

…risterAdd Natvis visualiser and debuginfo tests for `f16`To render `f16`s in debuggers on MSVC targets, this PR changes the compiler to output `f16`s as `struct f16 { bits: u16 }`, and includes a Natvis visualiser that manually converts the `f16`'s bits to a `float` which is can then be displayed by debuggers. `gdb`, `lldb` and `cdb` tests are also included for `f16` .`f16`/`f128` MSVC debug info issue:rust-lang#121837Tracking issue:rust-lang#116909

This comment has been minimized.

Copy link

Collaborator

bors commentedJul 9, 2024

💔 Test failed -checks-actions

Jul 9, 2024

Add Natvis visualiser and debuginfo tests forf16

b058de9

beetrees force-pushed thef16-debuginfo branch from4252842 tob058de9Compare

July 9, 2024 02:50

Copy link

ContributorAuthor

beetrees commentedJul 9, 2024

I've fixed the typos in the test that caused the failure.

Copy link

Member

michaelwoerister commentedJul 9, 2024

@bors r+

Copy link

Collaborator

bors commentedJul 9, 2024

📌 Commitb058de9 has been approved bymichaelwoerister

It is now in thequeue for this repository.

Jul 9, 2024

Copy link

Collaborator

bors commentedJul 9, 2024

⌛ Testing commitb058de9 with merge8672b2b...

Copy link

Collaborator

bors commentedJul 9, 2024

☀️ Test successful -checks-actions
Approved by: michaelwoerister
Pushing8672b2b to master...

bors added the merged-by-borsThis PR was explicitly merged by bors. label

Jul 9, 2024

bors merged commit8672b2b intorust-lang:master

Jul 9, 2024

rustbot added this to the1.81.0 milestone

Jul 9, 2024

Copy link

Collaborator

rust-timer commentedJul 9, 2024

Finished benchmarking commit (8672b2b):comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results (secondary -4.7%)

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)	-	-	0
Improvements ✅ (secondary)	-4.7%	[-4.7%, -4.7%]	1
All ❌✅ (primary)	-	-	0

Cycles

Results (primary -3.5%)

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.5%	[-3.5%, -3.5%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-3.5%	[-3.5%, -3.5%]	1