cg_llvm: Replace some DIBuilder wrappers with LLVM-C API bindings (part 2) #136632
Conversation
Zalathar
added
the
A-LLVM
This comment has been minimized.
This comment has been minimized.
|
Looks like I discovered an existing bug by accidentally fixing it. In #116096, the intent was to tell debuginfo that function ZSTs have a size of 0 bytes and an alignment of 1 byte. But because debuginfo typically handles alignment in bits, the alignment was accidentally set to 1 bit instead. (It doesn't help that LLVM IR just prints “align” without specifying a unit, so |
…ompiler-errors Debuginfo for function ZSTs should have alignment of 8 bits, not 1 bit In rust-lang#116096, function ZSTs were made to have debuginfo that gives them an alignment of “1”. But because alignment in LLVM debuginfo is denoted in *bits*, not bytes, this resulted in an alignment specification of 1 bit instead of 1 byte. I don't know whether this has any practical consequences, but I noticed that a test started failing when I accidentally fixed the mistake while working on rust-lang#136632, so I extracted the fix (and the test adjustment) to this PR.
…ompiler-errors Debuginfo for function ZSTs should have alignment of 8 bits, not 1 bit In rust-lang#116096, function ZSTs were made to have debuginfo that gives them an alignment of “1”. But because alignment in LLVM debuginfo is denoted in *bits*, not bytes, this resulted in an alignment specification of 1 bit instead of 1 byte. I don't know whether this has any practical consequences, but I noticed that a test started failing when I accidentally fixed the mistake while working on rust-lang#136632, so I extracted the fix (and the test adjustment) to this PR.
Rollup merge of rust-lang#136640 - Zalathar:debuginfo-align-bits, r=compiler-errors Debuginfo for function ZSTs should have alignment of 8 bits, not 1 bit In rust-lang#116096, function ZSTs were made to have debuginfo that gives them an alignment of “1”. But because alignment in LLVM debuginfo is denoted in *bits*, not bytes, this resulted in an alignment specification of 1 bit instead of 1 byte. I don't know whether this has any practical consequences, but I noticed that a test started failing when I accidentally fixed the mistake while working on rust-lang#136632, so I extracted the fix (and the test adjustment) to this PR.
|
☔ The latest upstream changes (presumably #136728) made this pull request unmergeable. Please resolve the merge conflicts. |
bors
added
the
S-waiting-on-author
|
I'll come back to this after #136881 has had a chance to land. |
Zalathar
force-pushed
the
llvm-di-builder
branch
from
d9273a9 to
1686818
Compare
rustbot
added
the
T-compiler
rustbot
added
S-waiting-on-review
Zalathar
force-pushed
the
llvm-di-builder
branch
from
1686818 to
9b5ab3f
Compare
Zalathar
force-pushed
the
llvm-di-builder
branch
from
9b5ab3f to
5eecd47
Compare
|
Huh, somehow this never actually got assigned a reviewer. r? workingjubilee (or reroll) This is low-priority cleanup, so no particular rush. (Also this will probably conflict with #137247 at some point.) |
There was a problem hiding this comment.
this is where all the action is, then, since this makes-safe a few things...
| @@ -1772,6 +1772,59 @@ unsafe extern "C" { | |||
| Scope: &'ll Metadata, | |||
| InlinedAt: Option<&'ll Metadata>, | |||
| ) -> &'ll Metadata; | |||
|
|
|||
| pub(crate) fn LLVMDIBuilderCreateSubroutineType<'ll>( | |||
There was a problem hiding this comment.
trivially obvious but this can in fact do a UB if we violate it
| pub(crate) fn LLVMDIBuilderCreateSubroutineType<'ll>( | |
| /// create debuginfo for subroutine types | |
| /// # Safety | |
| /// - `ParameterTypes` and `NumParameterTypes` must be from the same slice | |
| pub(crate) fn LLVMDIBuilderCreateSubroutineType<'ll>( |
| Flags: DIFlags, // (optional; default is `DIFlags::FlagZero`) | ||
| ) -> &'ll Metadata; | ||
|
|
||
| pub(crate) fn LLVMDIBuilderCreateUnionType<'ll>( |
There was a problem hiding this comment.
same comment except for Name and NameLen, Elements and ElementsLen, UniqueId and UniqueIdLen, also please note whether the obvious should be a CStr with or without its nul byte
| pub(crate) fn LLVMDIBuilderCreateArrayType<'ll>( | ||
| Builder: &DIBuilder<'ll>, | ||
| Size: u64, | ||
| AlignInBits: u32, | ||
| Ty: &'ll Metadata, | ||
| Subscripts: *const &'ll Metadata, | ||
| NumSubscripts: c_uint, | ||
| ) -> &'ll Metadata; | ||
|
|
||
| pub(crate) fn LLVMDIBuilderCreateBasicType<'ll>( | ||
| Builder: &DIBuilder<'ll>, | ||
| Name: *const c_uchar, | ||
| NameLen: size_t, | ||
| SizeInBits: u64, | ||
| Encoding: c_uint, // `LLVMDWARFTypeEncoding` | ||
| Flags: DIFlags, // (optional; default is `DIFlags::FlagZero`) | ||
| ) -> &'ll Metadata; | ||
|
|
||
| pub(crate) fn LLVMDIBuilderCreatePointerType<'ll>( |
| unsafe { | ||
| llvm::LLVMDIBuilderCreateSubroutineType( | ||
| self, |
There was a problem hiding this comment.
Okay, so, it's somewhat hard to audit the safe wrappers if I don't know what safety constraints they are filling. I can guess, but I shouldn't have to. Either there should be safety comments on the wrapper decls or they should be on this block, so that future additions to these functions don't fuck up and change the parts of the code that have to be a certain way.
There was a problem hiding this comment.
like it could be, idk, instead of exhaustively listing param relationships
// SAFETY: this call is safe if we know the DIBuilder is live, which we do because
// we have it by-ref, and the rest is just translating slices to two params
rustbot
added
S-waiting-on-author
The bad news is that somebody has to guess, because LLVM doesn't document its API soundness conditions, and the hundreds of ad-hoc unsafe blocks in cg_llvm don't document their assumptions either. I'm sympathetic to wanting to make these bindings more auditable (since I had to do my best to “audit” each one while making these changes), but practically speaking I don't think it's realistic for individual bindings/wrappers to have a SAFETY comment more detailed than “please see the general safety remarks in the module docs”. (With the caveat that those docs mostly don't exist yet!) At the very least, any safety notes that we do add should probably be honest about how YOLO they actually are, because overconfident safety docs can be worse than no safety docs at all. |
|
@Zalathar well... I think that saying both the pointer and the len should come from the same slice is a pretty safe bet? 🤔 but yes you are right, please feel free to add this to any list of safety requirements: /// - lol who knows? even LLVM doesn't
|
|
to rephrase my "I shouldn't have to guess" thing: we should write down our guesses so we don't have to go through the process of resynthesizing the guesses each time. |
cg_llvmdebuginfo handling to use the LLVM-C API #134001This is the next batch of LLVMDIBuilder binding migrations.
This time I also introduced some safe wrapper methods, because I noticed a few cases where I was repeating the same irrelevant/default argument values at multiple call sites.