Better document Direct/Indirect locations. #1538
Conversation
yksmp/src/lib.rs
Outdated
| /// dereferencing LLVM just records the value as an (offset, register) pair where the register | ||
| /// is typically the base pointer: | ||
| /// * `u16`: Dwarf register number | ||
| /// The live variable's value is a pointer that point into the stack. Pointer values may be |
There was a problem hiding this comment.
I think the first sentence is sort-of correct, but it's also what has led both of us up the garden path.
I think the first thing to do is to put Indirect before Direct in the enum, since Indirect is the general case. Then Direct can say something like "Direct is an optimisation for those Indirect values that are themselves pointers to things on the stack. In other words, if an Indirect value is a pointer, then we can skip a dereference, and turn that Indirect into a Direct". [That's not very good, but hopefully you get the idea!]
I also think that one more more examples -- even brief sketches -- will help. The problem here is that it's really easy to misread what "pointer of pointer" and "value" and whatnot are. Thanks to your explanation yesterday, I (think I) now understand, but without that, I don't think the text here would quite be enough to kick my brain in the right direction.
yksmp/src/lib.rs
Outdated
| /// out the value the address described by the (offset, register) pair needs to be dereferenced | ||
| /// first. | ||
| /// The live variable lives on the stack. It may have been put there directly via an `alloca` | ||
| /// or has been spilled. Similar to `Direct`, the location is encoded as an offset relative to |
There was a problem hiding this comment.
I would drop "similar to Direct" because Indirect is the general case.
|
Please force push updates if you think it sensible based on my questions / comments. |
|
Try this! |
|
Perfect, thanks! |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
While we are here add an assertion that's been on my mind and is loosely related to direct locations.
@ltratt I'm not sure if you wanted more documentation here, but my mind is blank. So please let me know if you think there's more that can be added here.