Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[LLD/PDB] Output actual records to the globals stream

Authored by zturner on Aug 9 2017, 10:53 AM.



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.

Diff Detail


Event Timeline

zturner created this revision.Aug 9 2017, 10:53 AM
rnk added inline comments.Aug 9 2017, 11:51 AM
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.

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.

zturner updated this revision to Diff 110469.Aug 9 2017, 1:37 PM

Made getting symbol name faster.

rnk added inline comments.Aug 9 2017, 1:46 PM
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').

rnk accepted this revision.Aug 9 2017, 3:55 PM


This revision is now accepted and ready to land.Aug 9 2017, 3:55 PM
zturner updated this revision to Diff 110504.Aug 9 2017, 4:50 PM

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.

rnk added inline comments.Aug 9 2017, 5:46 PM
476 ↗(On Diff #110504)

We don't seem to have test coverage for this. Do you mind adding one? You might be able to modify one of the yaml test cases...

57 ↗(On Diff #110504)

Oh right, these do only go in the globals section. Woops.

zturner updated this revision to Diff 110624.Aug 10 2017, 12:46 PM

Added a test for all global symbol types.

This revision was automatically updated to reflect the committed changes.