Page MenuHomePhabricator

[clang-format] Assert that filenames are not empty
ClosedPublic

Authored by MyDeveloperDay on Jan 4 2019, 5:37 PM.

Details

Summary

Adds asserts to catch empty filenames, which otherwise will cause a crash in SourceManager.
The clang-format tool now outputs an error if an empty filename is used.
Fixes bug: 34667

Diff Detail

Event Timeline

jr created this revision.Jan 4 2019, 5:37 PM
MyDeveloperDay added a project: Restricted Project.Oct 11 2019, 2:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 11 2019, 2:41 AM
MyDeveloperDay accepted this revision.Oct 11 2019, 6:08 AM

Thanks for the patch, I'm sorry it's taken so long sometimes items get lost...This looks to still be relevant, I think you'll need to rebase but it LGTM

$ clang-format --assume-filename= < ../clang-format-examples/test.cpp
Assertion failed: !HasError && "Cannot get value when an error exists!", file C:\llvm/llvm-project/llvm/include\llvm/Support/ErrorOr.h, line 243
Stack dump:
0.      Program arguments: C:\llvm\build\bin\clang-format.exe --assume-filename=
 #0 0x00007ff64d041b55 (C:\llvm\build\bin\clang-format.exe+0x41b55)
 #1 0x00007ff8efdcda2d (C:\WINDOWS\System32\ucrtbase.dll+0x6da2d)
 #2 0x00007ff8efdce901 (C:\WINDOWS\System32\ucrtbase.dll+0x6e901)
 #3 0x00007ff8efdd0261 (C:\WINDOWS\System32\ucrtbase.dll+0x70261)
 #4 0x00007ff8efdd0591 (C:\WINDOWS\System32\ucrtbase.dll+0x70591)
 #5 0x00007ff64d065cdd (C:\llvm\build\bin\clang-format.exe+0x65cdd)
 #6 0x00007ff64d0a11f3 (C:\llvm\build\bin\clang-format.exe+0xa11f3)
 #7 0x00007ff64d081dc3 (C:\llvm\build\bin\clang-format.exe+0x81dc3)
 #8 0x00007ff64d0817bb (C:\llvm\build\bin\clang-format.exe+0x817bb)
 #9 0x00007ff64d00b17c (C:\llvm\build\bin\clang-format.exe+0xb17c)
#10 0x00007ff64d00d515 (C:\llvm\build\bin\clang-format.exe+0xd515)
#11 0x00007ff64d12cc30 (C:\llvm\build\bin\clang-format.exe+0x12cc30)
#12 0x00007ff8efe77974 (C:\WINDOWS\System32\KERNEL32.DLL+0x17974)
#13 0x00007ff8f2caa271 (C:\WINDOWS\SYSTEM32\ntdll.dll+0x6a271)
This revision is now accepted and ready to land.Oct 11 2019, 6:08 AM
MyDeveloperDay commandeered this revision.Wed, Nov 6, 1:01 AM
MyDeveloperDay edited reviewers, added: jr; removed: MyDeveloperDay.

@jr As there is no response from you, I wonder if you are ok with me taking this over, I'd like to recommend we keep just the:

if (AssumedFileName.empty()) {
    llvm::errs() << "error: empty filenames are not allowed\n";
    return true;
  }

For now, as the other functions where you are adding an assert might be being called from other tooling areas and I don't want to cause others problems, I'd like to keep this change localized, (I hope it's ok but I'll rebase and update the patch)

This revision now requires review to proceed.Wed, Nov 6, 1:01 AM
MyDeveloperDay added a reviewer: mitchell-stellar.

remove the asserts at least for now.

This fix is important for people using clang-format integrated into some editor tools.

This revision is now accepted and ready to land.Wed, Nov 6, 5:28 AM
This revision was automatically updated to reflect the committed changes.