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

attempt to support BinaryFormat::Xcoff in naked_asm! #137816

Merged
merged 2 commits into from
Mar 13, 2025

Conversation

folkertdev
Copy link
Contributor

Fixes #137219

So, the inline assembly support for xcoff is extremely limited. The LLVM XCOFFAsmParser does not support many of the attributes that LLVM itself emits, and that should exist based on the assembler docs. It also does accept some that should not exist based on those docs.

So, I've tried to do the best I can given those limitations. At least it's better than emitting the directives for elf and having that fail somewhere deep in LLVM. Given that inline assembly for this target is incomplete (under asm_experimental_arch), I think that's OK (and again I don't see how we can do better given the limitations in LLVM).

r? @Noratrieb (given that you reviewed #136637)

It seems reasonable to ping the powerpc64-ibm-aix target maintainers, hopefully they have thoughts too: @daltenty @gilamn5tr

@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 Feb 28, 2025
Copy link
Contributor

@daltenty daltenty left a comment

Choose a reason for hiding this comment

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

Thanks for attempting this. Yeah, the XCOFF assembly parser is indeed has a bunch of problems, we never implemented a lot of the directives you find in normal AIX assembly.

The general approach looks reasonable enough to me, some minor comments

//
// Consequently, we try our best here but cannot do as good a job as for other targets.

// FIXME: start a section. `.csect` is not currently implemented in LLVM
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, we modelled the CSect as MCSections in the LLVM backend (XCOFF doesn't really have arbitrary sections, so that works well enought). So for the assembly writing the .csect directive is written when we switch to section.

With that in mind I wonder if .section works for the parser? Otherwise, it's not clear to me which .csect this ends up in? (Without -ffunction-sections we typical have a catch all ..text..[PR], if it's that one seems fine)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't look like it https://godbolt.org/z/4zfvahexs, neither .section or .pushsection is recognized.

// fun fact: according to the assembler documentation, .align takes an exponent,
// but LLVM only accepts powers of 2 (but does emit the exponent)
// so when we hand `.align 32` to LLVM, the assembly output will contain `.align 5`
writeln!(begin, ".align {}", align_bytes).unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

note: Yeah, this is a fun dialect difference of AIX assembly that we don't handle well

Copy link
Contributor Author

@folkertdev folkertdev left a comment

Choose a reason for hiding this comment

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

thanks for taking a look!

//
// Consequently, we try our best here but cannot do as good a job as for other targets.

// FIXME: start a section. `.csect` is not currently implemented in LLVM
Copy link
Contributor Author

Choose a reason for hiding this comment

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

doesn't look like it https://godbolt.org/z/4zfvahexs, neither .section or .pushsection is recognized.

Comment on lines 165 to 170
// FIXME: .weak_definition is accepted by the assembly parser, but is not
// documented https://www.ibm.com/docs/en/ssw_aix_71/assembler/assembler_pdf.pdf
// For all I know, it might just be ignored.
//
// On the other hand `.weak` is documented, but rejected as an "unknown directive".
writeln!(w, ".weak_definition {asm_name}")?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@daltenty do you have thoughts here? Leaving the fixme is probably fine for now, I'm guessing the .weak_definition is somehow supported by the base asm parser implementation? Do you know whether this has any effect?

Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately this doesn't have an effect it seems, and it crashes the backend if you actually try to emit and object: https://godbolt.org/z/rderToPx4

Not sure what we can do in this case, we may just be missing too much of the LLVM side at the moment unfortunately.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well I'll just make this generate a decent error message then saying that it's not currently supported, and link to an llvm issue to track

@rustbot
Copy link
Collaborator

rustbot commented Mar 7, 2025

Some changes occurred in compiler/rustc_codegen_ssa

cc @WaffleLapkin

@folkertdev
Copy link
Contributor Author

Allright, I think I at least have a good picture of the limitations now (hopefully they are documented well) and this PR tests for some of the weird edge cases (e.g. how alignment is handled), in case those change. So, I think this is the best we can to today, and handling of xcoff can be improved when LLVM improves.

@Noratrieb
Copy link
Member

looks good (or well, as good as it can look given the bad assembly support :D).
it's just a tier 3 target, and a target maintainer reviewed it as well.
@bors r+

@bors
Copy link
Contributor

bors commented Mar 12, 2025

📌 Commit f35bda3 has been approved by Noratrieb

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 Mar 12, 2025
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 13, 2025
…trieb

attempt to support `BinaryFormat::Xcoff` in `naked_asm!`

Fixes rust-lang#137219

So, the inline assembly support for xcoff is extremely limited. The LLVM [XCOFFAsmParser](https://github.com/llvm/llvm-project/blob/1b25c0c4da968fe78921ce77736e5baef4db75e3/llvm/lib/MC/MCParser/XCOFFAsmParser.cpp) does not support many of the attributes that LLVM itself emits, and that should exist based on [the assembler docs](https://www.ibm.com/docs/en/ssw_aix_71/assembler/assembler_pdf.pdf). It also does accept some that should not exist based on those docs.

So, I've tried to do the best I can given those limitations. At least it's better than emitting the directives for elf and having that fail somewhere deep in LLVM. Given that inline assembly for this target is incomplete (under `asm_experimental_arch`), I think that's OK (and again I don't see how we can do better given the limitations in LLVM).

r? `@Noratrieb` (given that you reviewed rust-lang#136637)

It seems reasonable to ping the [`powerpc64-ibm-aix` target maintainers](https://doc.rust-lang.org/rustc/platform-support/aix.html), hopefully they have thoughts too: `@daltenty` `@gilamn5tr`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 13, 2025
…trieb

attempt to support `BinaryFormat::Xcoff` in `naked_asm!`

Fixes rust-lang#137219

So, the inline assembly support for xcoff is extremely limited. The LLVM [XCOFFAsmParser](https://github.com/llvm/llvm-project/blob/1b25c0c4da968fe78921ce77736e5baef4db75e3/llvm/lib/MC/MCParser/XCOFFAsmParser.cpp) does not support many of the attributes that LLVM itself emits, and that should exist based on [the assembler docs](https://www.ibm.com/docs/en/ssw_aix_71/assembler/assembler_pdf.pdf). It also does accept some that should not exist based on those docs.

So, I've tried to do the best I can given those limitations. At least it's better than emitting the directives for elf and having that fail somewhere deep in LLVM. Given that inline assembly for this target is incomplete (under `asm_experimental_arch`), I think that's OK (and again I don't see how we can do better given the limitations in LLVM).

r? ``@Noratrieb`` (given that you reviewed rust-lang#136637)

It seems reasonable to ping the [`powerpc64-ibm-aix` target maintainers](https://doc.rust-lang.org/rustc/platform-support/aix.html), hopefully they have thoughts too: ``@daltenty`` ``@gilamn5tr``
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 13, 2025
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#137816 (attempt to support `BinaryFormat::Xcoff` in `naked_asm!`)
 - rust-lang#138109 (make precise capturing args in rustdoc Json typed)
 - rust-lang#138343 (Enable `f16` tests for `powf`)
 - rust-lang#138356 (bump libc to 0.2.171 to fix xous)
 - rust-lang#138371 (Update compiletest's `has_asm_support` to match rustc)
 - rust-lang#138380 (ci: add runners for vanilla LLVM 20)
 - rust-lang#138404 (Cleanup sysroot locating a bit)

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

Rollup of 6 pull requests

Successful merges:

 - rust-lang#137816 (attempt to support `BinaryFormat::Xcoff` in `naked_asm!`)
 - rust-lang#138109 (make precise capturing args in rustdoc Json typed)
 - rust-lang#138343 (Enable `f16` tests for `powf`)
 - rust-lang#138356 (bump libc to 0.2.171 to fix xous)
 - rust-lang#138371 (Update compiletest's `has_asm_support` to match rustc)
 - rust-lang#138404 (Cleanup sysroot locating a bit)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 762acf5 into rust-lang:master Mar 13, 2025
6 checks passed
@rustbot rustbot added this to the 1.87.0 milestone Mar 13, 2025
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 13, 2025
Rollup merge of rust-lang#137816 - folkertdev:naked-asm-xcoff, r=Noratrieb

attempt to support `BinaryFormat::Xcoff` in `naked_asm!`

Fixes rust-lang#137219

So, the inline assembly support for xcoff is extremely limited. The LLVM [XCOFFAsmParser](https://github.com/llvm/llvm-project/blob/1b25c0c4da968fe78921ce77736e5baef4db75e3/llvm/lib/MC/MCParser/XCOFFAsmParser.cpp) does not support many of the attributes that LLVM itself emits, and that should exist based on [the assembler docs](https://www.ibm.com/docs/en/ssw_aix_71/assembler/assembler_pdf.pdf). It also does accept some that should not exist based on those docs.

So, I've tried to do the best I can given those limitations. At least it's better than emitting the directives for elf and having that fail somewhere deep in LLVM. Given that inline assembly for this target is incomplete (under `asm_experimental_arch`), I think that's OK (and again I don't see how we can do better given the limitations in LLVM).

r? ```@Noratrieb``` (given that you reviewed rust-lang#136637)

It seems reasonable to ping the [`powerpc64-ibm-aix` target maintainers](https://doc.rust-lang.org/rustc/platform-support/aix.html), hopefully they have thoughts too: ```@daltenty``` ```@gilamn5tr```
github-actions bot pushed a commit to model-checking/verify-rust-std that referenced this pull request Mar 14, 2025
…iaskrgr

Rollup of 6 pull requests

Successful merges:

 - rust-lang#137816 (attempt to support `BinaryFormat::Xcoff` in `naked_asm!`)
 - rust-lang#138109 (make precise capturing args in rustdoc Json typed)
 - rust-lang#138343 (Enable `f16` tests for `powf`)
 - rust-lang#138356 (bump libc to 0.2.171 to fix xous)
 - rust-lang#138371 (Update compiletest's `has_asm_support` to match rustc)
 - rust-lang#138404 (Cleanup sysroot locating a bit)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Case for Xcoff not handled in prefix_and_suffix
6 participants