Pass 128-bit C-style enum enumerator values to LLVM #102717

beetrees · 2022-10-05T19:32:38Z

Pass the full 128 bits of C-style enum enumerators through to LLVM. This means that debuginfo for C-style repr128 enums is now emitted correctly for DWARF platforms (as compared to not being correctly emitted on any platform).

Tracking issue: #56071

rust-highfive · 2022-10-05T19:32:41Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @jackh726 (or someone else) soon.

Please see the contribution instructions for more information.

est31 · 2022-10-05T21:44:44Z

This probably fixes #100612? If so, can you add the regression test mentioned in the bug report?

beetrees · 2022-10-05T21:57:37Z

Unfortunately it does not: the ICE in #100612 is caused by the panic here in the code handling enums that have variants with fields, whereas this PR is for the code handling enums without any fields (C-style enums). While it is currently possible to pass the full 128 bits to LLVM for enums with fields too, LLVM currently truncates the discriminants of variants in debuginfo to 64 bits here, whereas C-style enumerators are not truncated by LLVM when emitting DWARF.

est31 · 2022-10-05T22:10:49Z

I see. It would be helpful though to have some kind of test.

beetrees · 2022-10-05T22:36:52Z

I agree, however I'm not sure what the best way to test it is. While this PR means that the DWARF is correctly emitted, neither of the two DWARF debuggers that are currently tested support 128-bit enums, with GDB stating error reading variable: That operation is not available on integers of more than 8 bytes. (this error also appears when trying to debug the stable Option<NonZeroU128>), and LLDB giving an assertion error GetMaxU64 invalid byte_size! (the same errors occur when trying to debug enums in C++ using __uint128_t as the discriminant type). The best idea I currently have is to add a test that uses the llvm-dwarfdump command to check that the enumerators are emitted correctly in the DWARF.

est31 · 2022-10-06T00:42:41Z

Makes sense. I guess one can do as you suggest. IDK a test is not that important, but having one would still be preferable so that nothing regresses.

beetrees · 2022-10-06T19:28:33Z

I've added a run-make test that uses llvm-dwarfdump.

est31 · 2022-10-06T19:45:49Z

@beetrees thanks for that!

beetrees · 2022-10-09T08:36:18Z

I've updated the PR to remove an outdated comment.

jackh726 · 2022-10-24T00:08:59Z

Not my area of expertise

r? rust-lang/compiler

fee1-dead · 2022-10-24T03:59:24Z

Not really mine either.

r? compiler

compiler-errors · 2022-11-08T18:31:27Z

This looks right to me, but anyone from @rust-lang/wg-llvm can comment on the validity of the code?

nagisa

Really nice cleanup too!

nagisa · 2022-11-08T18:43:33Z

src/test/run-make/repr128-dwarf/Makefile

+	$(RUSTC) -Cdebuginfo=2 lib.rs -o $(TMPDIR)/repr128.rlib
+	"$(LLVM_BIN_DIR)"/llvm-dwarfdump -n U128A $(TMPDIR)/repr128.rlib | $(CGREP) "DW_AT_const_value	(<0x10> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 )"
+	"$(LLVM_BIN_DIR)"/llvm-dwarfdump -n U128B $(TMPDIR)/repr128.rlib | $(CGREP) "DW_AT_const_value	(<0x10> 01 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 )"
+	"$(LLVM_BIN_DIR)"/llvm-dwarfdump -n U128C $(TMPDIR)/repr128.rlib | $(CGREP) "DW_AT_const_value	(<0x10> 00 00 00 00 00 00 00 00 01 00 00 00 00 00 00 00 )"
+	"$(LLVM_BIN_DIR)"/llvm-dwarfdump -n U128D $(TMPDIR)/repr128.rlib | $(CGREP) "DW_AT_const_value	(<0x10> ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff )"
+	"$(LLVM_BIN_DIR)"/llvm-dwarfdump -n I128A $(TMPDIR)/repr128.rlib | $(CGREP) "DW_AT_const_value	(<0x10> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 )"
+	"$(LLVM_BIN_DIR)"/llvm-dwarfdump -n I128B $(TMPDIR)/repr128.rlib | $(CGREP) "DW_AT_const_value	(<0x10> ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff )"
+	"$(LLVM_BIN_DIR)"/llvm-dwarfdump -n I128C $(TMPDIR)/repr128.rlib | $(CGREP) "DW_AT_const_value	(<0x10> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 80 )"
+	"$(LLVM_BIN_DIR)"/llvm-dwarfdump -n I128D $(TMPDIR)/repr128.rlib | $(CGREP) "DW_AT_const_value	(<0x10> ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff 7f )"


Rather than this (or in addition to this) lets add a debuginfo test instead. It doesn't matter particularly much what dwarfdump outputs if it turns out that the primary consumers of this data (gdb/lldb/other debuggers) can’t really stomach this kind of data anyway. E.g. if those “just” crash, then perhaps the ecosystem just isn’t ready for this and we probably shouldn't be stabilizing this feature either way.

gdb and lldb don't currently crash, but neither give any useful output that could be tested for (gdb gives a nice error message instead of the current value of the enum-typed variable, lldb prints an internal assertion error and doesn't correctly process the enum-typed variable in question but otherwise keeps going). This problem with debuggers can already be exposed using stable rustc with a variable of type Option<NonZeroU128>, which has a 128-bit discriminant (gdb gives the same error message, lldb doesn't support enums with fields at all).

Since gdb and lldb don't currently support 128-bit enums, this test seems like the best option to make sure the DWARF output doesn't break. I agree that once gdb or lldb gain support for 128-bit enums, this test should be replaced with a regular debuginfo test.

nagisa · 2022-11-08T18:52:41Z

compiler/rustc_codegen_llvm/src/llvm/ffi.rs

@@ -2118,7 +2118,8 @@ extern "C" {
        Builder: &DIBuilder<'a>,
        Name: *const c_char,
        NameLen: size_t,
-        Value: i64,
+        Value: *const u64,
+        SizeInBits: c_uint,


Any reason to prefer c_uint over u64 like in the function above?

unsigned int is the type of the SizeInBits parameter of the APInt constructor on the C++ side; using c_uint here means any integer truncation is visible on the Rust side rather than buried in the C++ wrapper (this shouldn't matter in practice as the maximum value of SizeInBits here is 128). I'm happy to change it to u64 if you think that would be an improvement, but using c_uint here seems like better design.

nagisa · 2022-11-08T18:54:24Z

r? @nagisa

nagisa · 2022-11-20T20:58:02Z

@bors r+

Thanks!

bors · 2022-11-20T20:58:04Z

📌 Commit 6f6eebd has been approved by nagisa

It is now in the queue for this repository.

nagisa · 2022-11-20T20:58:35Z

@bors rollup=never

bors · 2022-11-20T22:27:23Z

⌛ Testing commit 6f6eebd with merge dd0fe1b3541ae0002b29fa12c9138e2d44a5b662...

bors · 2022-11-20T22:30:52Z

💔 Test failed - checks-actions

rust-log-analyzer · 2022-11-20T22:38:16Z

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

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

nagisa · 2022-11-21T01:26:04Z

@bors retry network error

bors · 2022-11-21T01:44:16Z

⌛ Testing commit 6f6eebd with merge ccde51a...

bors · 2022-11-21T04:42:37Z

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing ccde51a to master...

rust-timer · 2022-11-21T06:03:47Z

Finished benchmarking commit (ccde51a): comparison URL.

Overall result: ❌ regressions - 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.7%	[0.5%, 0.9%]	7
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.7%	[0.5%, 0.9%]	7

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
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-5.2%	[-5.2%, -5.2%]	1
Improvements ✅ (secondary)	-3.2%	[-3.8%, -2.6%]	2
All ❌✅ (primary)	-5.2%	[-5.2%, -5.2%]	1

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)	3.6%	[2.1%, 5.1%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

lqd · 2022-11-21T08:44:39Z

This is recent bitmaps noise: @rustbot label: +perf-regression-triaged

…r=nagisa Pass 128-bit C-style enum enumerator values to LLVM Pass the full 128 bits of C-style enum enumerators through to LLVM. This means that debuginfo for C-style repr128 enums is now emitted correctly for DWARF platforms (as compared to not being correctly emitted on any platform). Tracking issue: rust-lang#56071

rust-highfive assigned jackh726 Oct 5, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 5, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 5, 2022

beetrees added 2 commits October 9, 2022 09:33

Pass 128-bit C-style enum enumerator values to LLVM

25c1c63

Add a test for C-style repr128 enum DWARF debuginfo

6f6eebd

beetrees force-pushed the repr128-c-style-debuginfo branch from 6fa3354 to 6f6eebd Compare October 9, 2022 08:35

rust-highfive assigned fee1-dead and unassigned jackh726 Oct 24, 2022

rust-highfive assigned compiler-errors and unassigned fee1-dead Oct 24, 2022

nagisa reviewed Nov 8, 2022

View reviewed changes

rustbot assigned nagisa and unassigned compiler-errors Nov 8, 2022

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 Nov 20, 2022

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 Nov 20, 2022

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 Nov 21, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 21, 2022

bors merged commit ccde51a into rust-lang:master Nov 21, 2022

rustbot added this to the 1.67.0 milestone Nov 21, 2022

bors mentioned this pull request Nov 21, 2022

Require type_map::stub callers to supply file information #104342

Merged

rustbot added the perf-regression Performance regression. label Nov 21, 2022

rustbot added the perf-regression-triaged The performance regression has been triaged. label Nov 21, 2022

beetrees deleted the repr128-c-style-debuginfo branch March 30, 2023 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass 128-bit C-style enum enumerator values to LLVM #102717

Pass 128-bit C-style enum enumerator values to LLVM #102717

beetrees commented Oct 5, 2022

rust-highfive commented Oct 5, 2022

est31 commented Oct 5, 2022

beetrees commented Oct 5, 2022

est31 commented Oct 5, 2022

beetrees commented Oct 5, 2022

est31 commented Oct 6, 2022

beetrees commented Oct 6, 2022

est31 commented Oct 6, 2022

beetrees commented Oct 9, 2022

jackh726 commented Oct 24, 2022

fee1-dead commented Oct 24, 2022

compiler-errors commented Nov 8, 2022

nagisa left a comment •

edited

Loading

nagisa Nov 8, 2022 •

edited

Loading

beetrees Nov 10, 2022

nagisa Nov 8, 2022

beetrees Nov 10, 2022

nagisa commented Nov 8, 2022

nagisa commented Nov 20, 2022

bors commented Nov 20, 2022

nagisa commented Nov 20, 2022

bors commented Nov 20, 2022

bors commented Nov 20, 2022

rust-log-analyzer commented Nov 20, 2022

nagisa commented Nov 21, 2022

bors commented Nov 21, 2022

bors commented Nov 21, 2022

rust-timer commented Nov 21, 2022

lqd commented Nov 21, 2022

Pass 128-bit C-style enum enumerator values to LLVM #102717

Pass 128-bit C-style enum enumerator values to LLVM #102717

Conversation

beetrees commented Oct 5, 2022

rust-highfive commented Oct 5, 2022

est31 commented Oct 5, 2022

beetrees commented Oct 5, 2022

est31 commented Oct 5, 2022

beetrees commented Oct 5, 2022

est31 commented Oct 6, 2022

beetrees commented Oct 6, 2022

est31 commented Oct 6, 2022

beetrees commented Oct 9, 2022

jackh726 commented Oct 24, 2022

fee1-dead commented Oct 24, 2022

compiler-errors commented Nov 8, 2022

nagisa left a comment • edited Loading

Choose a reason for hiding this comment

nagisa Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

beetrees Nov 10, 2022

Choose a reason for hiding this comment

nagisa Nov 8, 2022

Choose a reason for hiding this comment

beetrees Nov 10, 2022

Choose a reason for hiding this comment

nagisa commented Nov 8, 2022

nagisa commented Nov 20, 2022

bors commented Nov 20, 2022

nagisa commented Nov 20, 2022

bors commented Nov 20, 2022

bors commented Nov 20, 2022

rust-log-analyzer commented Nov 20, 2022

nagisa commented Nov 21, 2022

bors commented Nov 21, 2022

bors commented Nov 21, 2022

rust-timer commented Nov 21, 2022

Overall result: ❌ regressions - ACTION NEEDED

Instruction count

Max RSS (memory usage)

Cycles

lqd commented Nov 21, 2022

nagisa left a comment •

edited

Loading

nagisa Nov 8, 2022 •

edited

Loading