This is an archive of the discontinued LLVM Phabricator instance.

[Windows] Use llvm-symbolizer before using dbghelp
ClosedPublic

Authored by rnk on Aug 5 2015, 4:24 PM.

Details

Summary

llvm-symbolizer understands both PDBs and DWARF, so it's a better bet if
it's available. It prints out the function parameter types and column
numbers, so I needed to churn the expected test output a bit.

This makes most of the llvm-symbolizer subprocessing code
target-independent. Pipes on all platforms use fd_t, and we can use the
portable ReadFromFile / WriteToFile wrappers in symbolizer_sanitizer.cc.
Only the pipe creation and process spawning is Windows-specific.

Please check that the libcdep layering is still correct. I don't know
how to reproduce the build configuration that relies on that.

Diff Detail

Repository
rL LLVM

Event Timeline

rnk updated this revision to Diff 31418.Aug 5 2015, 4:24 PM
rnk retitled this revision from to [Windows] Use llvm-symbolizer before using dbghelp.
rnk updated this object.
rnk added a reviewer: samsonov.
rnk added a subscriber: llvm-commits.
samsonov edited edge metadata.Aug 7 2015, 5:40 PM

Looks like some auxiliary NFC changes can be reviewed or landed separately to minimize the diff. As for libcdep/nolibc I think that "make check-all" on Linux should properly check for that.

lib/sanitizer_common/sanitizer_common.cc
347 ↗(On Diff #31418)

Move this function in a separate change.

355 ↗(On Diff #31418)

Use a named constant for pathsep

lib/sanitizer_common/sanitizer_symbolizer_win.cc
196 ↗(On Diff #31418)

Shouldn't you actually check for backslash?

242 ↗(On Diff #31418)

Does Windows properly handle common_flags()->symbolize? We shouldn't create any symbolizers if this flag is true.

test/asan/CMakeLists.txt
19 ↗(On Diff #31418)

Let's introduce COMPILER_RT_HAS_LLD_SOURCES in the same way we use COMPILER_RT_HAS_LIBCXX_SOURCES, and use a common variable to check if lld is available somewhere in test/lit.common.configured.in instead of ASAN_HAS_LLD

20 ↗(On Diff #31418)

Copy-paste error

test/asan/TestCases/Windows/operator_new_left_oob.cc
14 ↗(On Diff #31418)

Please relax all these test cases in a separate change.

rnk marked 4 inline comments as done.Aug 10 2015, 3:51 PM
rnk added inline comments.
lib/sanitizer_common/sanitizer_common.cc
347 ↗(On Diff #31418)
lib/sanitizer_common/sanitizer_symbolizer_win.cc
242 ↗(On Diff #31418)

Apparently not. I added that here.

test/asan/CMakeLists.txt
19 ↗(On Diff #31418)

OK

test/asan/TestCases/Windows/operator_new_left_oob.cc
14 ↗(On Diff #31418)
rnk updated this revision to Diff 31742.Aug 10 2015, 3:53 PM
rnk marked an inline comment as done.
rnk edited edge metadata.
  • Split stuff out and a few updates
rnk updated this revision to Diff 31745.Aug 10 2015, 4:22 PM
  • Fix gotsan build by moving symbolization to sanitizer_symbolize_libcdep.cc
samsonov added inline comments.Aug 10 2015, 5:03 PM
CMakeLists.txt
358 ↗(On Diff #31745)

Please land this (together with lit config and test/cfi changes) in a separate small commit.

lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc
306 ↗(On Diff #31745)

Why do you need to optionally return false (and fallback to the next symbolizer in chain here?

401 ↗(On Diff #31745)

Looks like just_read can't be -1 now, when we properly return the error from ReadFromFile function. Just delete this case.

420 ↗(On Diff #31745)

Same here: write_len can't be -1 if success is true. In fact, these changes (and CloseFile change above) are bugfixes, because checking for -1 was wrong in the first place - one should have called internal_iserror on the result of internal_write instead. I would appreciate if you will replace internal_read / internal_write / internal_close with ReadFromFile / WriteToFile / CloseFile as a separate small commit preceding this one.

lib/sanitizer_common/sanitizer_symbolizer_process_libcdep.cc
1 ↗(On Diff #31745)

This file makes little sense now, after you've moved significant part of SymbolizerProcess implementation to sanitizer_symbolizer_libcdep.cc. Probably, now you can move the only remaining function to sanitizer_symbolizer_posix_libcdep.cc, and delete this file?

rnk marked 3 inline comments as done.Aug 10 2015, 5:34 PM
rnk added inline comments.
CMakeLists.txt
358 ↗(On Diff #31745)
lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc
306 ↗(On Diff #31745)

I guess I don't anymore, now that the test case updates have landed. Previously I was trying to make it so that asan would use dbghelp for modules with PDBs and llvm-symbolizer for modules with DWARF. What I discovered was that llvm-symbolizer handles both PDBs and DWARF *and* provides column information, which dbghelp does not. At that point, I made the test case updates and reordered things to prefer llvm-symbolizer over dbghelp.

I probably don't need this functionality anymore, but it seems useful to me. llvm-symbolizer might not be able to answer a particular query, and this lets some other tool try.

420 ↗(On Diff #31745)
rnk updated this revision to Diff 31752.Aug 10 2015, 5:43 PM
rnk marked an inline comment as done.
  • Rebase
rnk added inline comments.Aug 10 2015, 5:45 PM
lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc
273–291 ↗(On Diff #31752)

This tokenization change here might be worth looking at closely, it's pretty horrible.

samsonov accepted this revision.Aug 10 2015, 6:06 PM
samsonov edited edge metadata.

LGTM. Please watch the bots after the commit. Thank you!

lib/sanitizer_common/sanitizer_symbolizer_libcdep.cc
273–291 ↗(On Diff #31752)

Yep, I've noticed this horridness. Might worth moving into a separate function btw.

283 ↗(On Diff #31752)

Why not ExtractInt(last_colon + 1, "", &info->line)?

306 ↗(On Diff #31752)

I see, but I'd rather avoid changing the behavior here. If llvm-symbolizer (on Linux/Mac) failed to get function name, it indeed probably means that there is no debug info, and no way for us to get meaningful information for stack trace - so why try the alternatives? If there *is* some debug info, and llvm-symbolizer weren't able to locate it - we want to actually learn about this and fix a bug.

This revision is now accepted and ready to land.Aug 10 2015, 6:06 PM
This revision was automatically updated to reflect the committed changes.
compiler-rt/trunk/lib/sanitizer_common/sanitizer_symbolizer_process_libcdep.cc