Previously we were just writing an empty globals stream. I hypothesized that fixing this would fix the last remaining known issue with using WinDbg, which is that we could not display local variables. Unfortunately this patch only gets us halfway there. Previously when displaying locals you would get an error message saying that private symbol information could not be found, and running the "lm" command in WinDbg would say that only public symbols were available. After this patch, the lm command reports that private symbols are present, but attempting to display a local variable still reports the same error. So there is something else missing.
Details
Diff Detail
- Repository
- rL LLVM
Event Timeline
llvm/lib/DebugInfo/CodeView/RecordName.cpp | ||
---|---|---|
280 ↗ | (On Diff #110437) | This isn't really "computing" the name, it just gets it. getSymbolName seems more appropriate. It should also return a StringRef. This also needs to be really fast, because we do it on every external symbol. Setting up a deserialization pipeline is probably too much work. Do any of these require any more work than knowing the offset of the name member of the symbol record? I'm thinking we might want to switch to compute the offset, and do the StringRef.split('\0') trick to find the null terminator. |
llvm/lib/DebugInfo/PDB/Native/GSIStreamBuilder.cpp | ||
95 ↗ | (On Diff #110437) | We do this a lot, this is going to happen once per record. It would be nice to avoid copying the bytes before we hash them. |
llvm/lib/DebugInfo/CodeView/RecordName.cpp | ||
---|---|---|
257–258 ↗ | (On Diff #110469) | LLVM frequently factors large switches like this into separate functions. In this case, you can return the offset or some sentinel value like 0 or -1. This saves duplication of Sym.content().drop_front(Offset) and break becomes return 35. |
325–327 ↗ | (On Diff #110469) | Ouch, I was hoping all names were at constant offsets. We can put this in a separate if to implement the previously suggested refactoring. |
342–343 ↗ | (On Diff #110469) | IMO it would be nicer to avoid the use of the lambda. We have alternatives like strnlen and .split('\0'). |
I had to edit a bunch of tests since we moved some symbols from module stream to globals stream. Re-uploading one more time to make sure that the changes to the test are sound.