Page MenuHomePhabricator

[clang-format] [RELAND] Remove the dependency on frontend
ClosedPublic

Authored by MyDeveloperDay on Nov 5 2019, 9:32 AM.

Details

Summary

relanding D68969: [clang-format] Remove the dependency on frontend after it failed UBSAN build caused by the passing of an invalid SMLoc() (nullptr)

Diff Detail

Event Timeline

MyDeveloperDay created this revision.Nov 5 2019, 9:32 AM

Build result: fail - 33803 tests passed, 1 failed and 462 were skipped.

failed: LLVM.Object/macho-invalid.test

Log files: console-log.txt, CMakeCache.txt

Build result: fail - 33803 tests passed, 1 failed and 462 were skipped.

failed: LLVM.Object/macho-invalid.test

Log files: console-log.txt, CMakeCache.txt

pretty sure this pre merge failure is unrelated.

thakis accepted this revision.Nov 6 2019, 5:46 AM

Thanks! One comment (the stale pointer one) and a more general suggestion (potentially for a future change) below, else I think this is good to go.

clang/tools/clang-format/ClangFormat.cpp
310

Maybe not for this change, but do you need all this machinery up here? SMDiagnostics takes a pointer and a file name, so yu can just pass Code->getBufferStart() + R.getOffset() for the pointer and you don't need the FileManager, the SourceManager, createInMemoryFile(), the PresumedLoc, translateFileLineCol.

You can either try to use SourceMgr::getLineAndColumn() or compute line/col yourself -- either should be a lot less code than the current machinery here.

335

I think the idea is that you have a llvm::SourceMgr() hanging around (above the loop or somewhere). SMDiagnostic stores a pointer to this argument, and you pass in a temporary. The SourceMgr needs to outlive the Diags object. (In practice, the pointer is not used by anything so it happens to work, but having the object store a stale pointer seems like potential future trouble, and it's easy to prevent -- just put the SourceMgr on the stack).

Also, probably "Diag", not "Diags"?

This revision is now accepted and ready to land.Nov 6 2019, 5:46 AM
MyDeveloperDay marked 5 inline comments as done.Nov 6 2019, 9:35 AM
MyDeveloperDay added inline comments.
clang/tools/clang-format/ClangFormat.cpp
310

So I definitely agree here, I'm afraid I shamelessly copied this from elsewhere.. but I see there should be a better way, let me work on that separately and if its ok I'll send a later review your way.

For now let me reland the frontEnd removal, with your suggested change before.

335

I made the suggested changes just prior to landing, hope that is ok.

This revision was automatically updated to reflect the committed changes.
MyDeveloperDay marked 2 inline comments as done.