The checksums had already been placed in the IR, this patch allows
MCCodeView to actually write it out to an MCStreamer.
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
| llvm/lib/MC/MCCodeView.cpp | ||
|---|---|---|
| 49 ↗ | (On Diff #114469) | remove |
| 63 ↗ | (On Diff #114469) | remove comment |
| 137 ↗ | (On Diff #114469) | combine this all into one line |
| 138 ↗ | (On Diff #114469) | Don't reassign to S and just directly use Ret.first |
| 155 ↗ | (On Diff #114469) | add a comment explaining this function |
| 156 ↗ | (On Diff #114469) | remove |
| 162 ↗ | (On Diff #114469) | remove spaces here |
| 168 ↗ | (On Diff #114469) | remove these |
| 174 ↗ | (On Diff #114469) | +=4 for the string table offset |
| 177 ↗ | (On Diff #114469) | +1 for size, +1 for kind, +2 to align (maybe just replace that with alignTo for clarity) |
| 180 ↗ | (On Diff #114469) | remove logging, more comments to explain offset |
| 216 ↗ | (On Diff #114469) | add comment explaining that, since the checksum section has been emitted, we can assume the offsets have been finalized so we can assign them. |
| 227 ↗ | (On Diff #114469) | update this comment |
| 231 ↗ | (On Diff #114469) | remove these |
| 241 ↗ | (On Diff #114469) | yes remove this |
| 253 ↗ | (On Diff #114469) | add comment to explain, how it needs to use a MCFixup provided by EmitValueImpl |
| 265 ↗ | (On Diff #114469) | remove |
| 268 ↗ | (On Diff #114469) | create a temporary symbol to hold the offset once it is calculated |
| 304 ↗ | (On Diff #114469) | remove |
| 319 ↗ | (On Diff #114469) | a |
| 458 ↗ | (On Diff #114469) | r |
| 504 ↗ | (On Diff #114469) | change this comment |
| llvm/lib/MC/MCObjectStreamer.cpp | ||
| 144 ↗ | (On Diff #114469) | r |
| llvm/lib/MC/MCParser/AsmParser.cpp | ||
| 3455 ↗ | (On Diff #114469) | should be lower case for consistency |
| llvm/lib/MC/WinCOFFObjectWriter.cpp | ||
| 725 ↗ | (On Diff #114469) | r |
| llvm/tools/llvm-readobj/COFFDumper.cpp | ||
| 910 ↗ | (On Diff #114469) | r these |
| 1020 ↗ | (On Diff #114469) | r |
| 1106 ↗ | (On Diff #114469) | not needed |
Comment Actions
Make this entire thing easier to follow and understand.
| llvm/lib/MC/MCCodeView.cpp | ||
|---|---|---|
| 60 ↗ | (On Diff #114469) | This is confusing to read, make it more intuitive |
Comment Actions
Why do we have to add placeholders for the checksum offsets? Aren't they just added in order, one after another?
Actually, the implementation of addFile hasn't assumed it is done in order, and you can actually add random fileNo's whenever you want. Therefore, we shouldn't assume they are always done in order, and we should allow for the case where we don't yet know the checksum offset.
| llvm/include/llvm/IR/DebugInfoMetadata.h | ||
|---|---|---|
| 476–481 ↗ | (On Diff #114116) | Unfortunately I need the actual enum values to correspond exactly to the checksum types flag integer values actually present in the codeview section, hence the strongly typed enum class. This is important because I will now be emitting the ChecksumKind as assembly directives as well as directly into the binary. |
| llvm/include/llvm/MC/MCCodeView.h | ||
| 297–300 ↗ | (On Diff #114116) | Yes, combined all this info into one vector. It's still useful to have a StringMap because we want to check if a filename is already used before making a new entry. |
| llvm/lib/MC/MCCodeView.cpp | ||
|---|---|---|
| 188–200 ↗ | (On Diff #114116) | It was all debugging code that was unintended for review. Please disregard. Sorry again for not making this patch private to begin with. |
| 418–420 ↗ | (On Diff #114116) | Filenames renamed to Files, which is now a vector of a massive structure containing all offset and checksum information. |
| llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp | ||
|---|---|---|
| 688 ↗ | (On Diff #115274) | Update this FIXME to say that we do provide checksum file info, and what VS uses this for. |
| llvm/lib/MC/MCCodeView.cpp | ||
|---|---|---|
| 169 ↗ | (On Diff #115274) | I don't think you want to do this, this leaks the internal MCSymbol (file1, etc) to the assembler. Nothing in the assembly defines file1, so the assembly isn't very readable. |
| llvm/test/DebugInfo/COFF/inlining.ll | ||
| 52 ↗ | (On Diff #115274) | What I had in mind was that we'd add a directive like .cv_filechecksum_offset <fileid> and MCCodeView.cpp would internally implement it without leaking any labels to the caller. We could use a label difference of two symbols to implement the offset computation, but any symbols shouldn't leak out to the assembler. |
| llvm/include/llvm/IR/DebugInfoMetadata.h | ||
|---|---|---|
| 476–481 ↗ | (On Diff #114116) | If you need the enums to have specific values, e.g. because those values end up in the object file (hard to tell here but I think that's what you are saying), you should set the enum values explicitly. This documents the exact values you need (even if they happen to be sequential at the moment). Making it an enum class doesn't get you that. |
| llvm/lib/IR/DebugInfoMetadata.cpp | ||
| 357 ↗ | (On Diff #115274) | If you require exact correspondence between an enum and some array of strings, we usually do that with a .def file and macros. See for example Dwarf.def. As it is, the association between this array and the ChecksumKind values is implied rather than explicit. Safer to be explicit. |
| llvm/lib/MC/MCCodeView.cpp | ||
|---|---|---|
| 68 ↗ | (On Diff #115274) | Use MCContext::createTempSymbol to make sure these don't show up in the .obj symbol table. |
| llvm/include/llvm/IR/DebugInfoMetadata.h | ||
|---|---|---|
| 476–481 ↗ | (On Diff #114116) | Remove enum class to explicitly define values. |
| llvm/lib/IR/DebugInfoMetadata.cpp | ||
| 357 ↗ | (On Diff #115274) | Okay this should probably be addressed in another patch as this patch did not introduce the array. |
| llvm/lib/MC/MCCodeView.cpp | ||
| 169 ↗ | (On Diff #115274) | Instead of emitting the symbol, we will emit the .cv_filechecksumoffset <fileno> directive. |
Comment Actions
A few minor commentary points; nothing huge. I've been looking at this mainly with the thought of having to do something similar for DWARF 5 eventually, but it seems likely there won't be a lot to leverage as CodeView and DWARF appear to associate the hashes with files pretty differently. Oh well.
| llvm/include/llvm/IR/DebugInfoMetadata.h | ||
|---|---|---|
| 476–481 ↗ | (On Diff #114116) | And maybe a comment that these end up in the object file, so some well-meaning person doesn't come a long later and remove the "unnecessary" explicit values. |
| llvm/include/llvm/MC/MCStreamer.h | ||
| 732 ↗ | (On Diff #115352) | As long as you're changing the comment anyway, please remove the \brief. |
| 778 ↗ | (On Diff #115352) | Remove \brief. |
| llvm/test/DebugInfo/COFF/inlining.ll | ||
|---|---|---|
| 52 ↗ | (On Diff #115274) | implemented as such |
Comment Actions
Thanks, looks good!
| llvm/include/llvm/MC/MCCodeView.h | ||
|---|---|---|
| 302–303 ↗ | (On Diff #115352) | Generally, if the comment describing the variable doesn't fit on one line, consider moving it before the variable. Then we won't end up with this awkard split after the *. |