[PATCH] D64428: Teach `llvm-pdbutil pretty -native` about `-injected-sources`

[George Rimar via Phabricator via llvm-commits]

grimar added inline comments.


================
Comment at: llvm/trunk/lib/DebugInfo/PDB/Native/NativeEnumInjectedSources.cpp:49
+    auto Name = Strings.getStringForID(Entry.FileNI);
+    assert(Name && "InjectedSourceStream should have rejected this");
+    return *Name;
----------------
grimar wrote:
> FWIW, it is not correct to handle LLVM errors like that.
> This code fails if the code is compiled with LLVM_ENABLE_ABI_BREAKING_CHECKS in Release (i.e. when asserts are disabled).
> 
> The problem is that `getStringForID` returns `Expected<StringRef>` and `Expected` value must always
> be checked, even if it is in success state.
> 
> Test failture log (found with check-llvm call):
> 
> ```
> ********************
> FAIL: LLVM :: tools/llvm-pdbutil/injected-sources-native.test (33257 of 33768)
> ******************** TEST 'LLVM :: tools/llvm-pdbutil/injected-sources-native.test' FAILED ********************
> Script:
> --
> : 'RUN: at line 4';   /home/umb/LLVM/build/bin/llvm-pdbutil pretty -native -injected-sources -injected-source-content    /home/umb/LLVM/llvm/test/tools/llvm-pdbutil/Inputs/InjectedSource.pdb | /home/umb/LLVM/build/bin/FileCheck /home/umb/LLVM/llvm/test/tools/llvm-pdbutil/injected-sources-native.test
> : 'RUN: at line 6';   /home/umb/LLVM/build/bin/llvm-pdbutil pretty -native -injected-sources -injected-source-content    /home/umb/LLVM/llvm/test/tools/llvm-pdbutil/Inputs/ClassLayoutTest.pdb | /home/umb/LLVM/build/bin/FileCheck --check-prefix=NEGATIVE /home/umb/LLVM/llvm/test/tools/llvm-pdbutil/injected-sources-native.test
> : 'RUN: at line 33';   /home/umb/LLVM/build/bin/llvm-pdbutil pretty -native -injected-sources -injected-source-content    /home/umb/LLVM/llvm/test/tools/llvm-pdbutil/Inputs/dotnet_hashonly.pdb | /home/umb/LLVM/build/bin/FileCheck --check-prefix=HASH /home/umb/LLVM/llvm/test/tools/llvm-pdbutil/injected-sources-native.test
> : 'RUN: at line 45';   /home/umb/LLVM/build/bin/llvm-pdbutil pretty -native -injected-sources -injected-source-content    /home/umb/LLVM/llvm/test/tools/llvm-pdbutil/Inputs/dotnet_contents_uncompressed.pdb | /home/umb/LLVM/build/bin/FileCheck --check-prefix=UNCOMP /home/umb/LLVM/llvm/test/tools/llvm-pdbutil/injected-sources-native.test
> : 'RUN: at line 62';   /home/umb/LLVM/build/bin/llvm-pdbutil pretty -native -injected-sources -injected-source-content    /home/umb/LLVM/llvm/test/tools/llvm-pdbutil/Inputs/dotnet_contents_compressed.pdb | /home/umb/LLVM/build/bin/FileCheck --check-prefix=COMP /home/umb/LLVM/llvm/test/tools/llvm-pdbutil/injected-sources-native.test
> --
> Exit Code: 2
> 
> Command Output (stderr):
> --
> Expected<T> must be checked before access or destruction.
> Expected<T> value was in success state. (Note: Expected<T> values in success mode must still be checked prior to being destroyed).
> Stack dump:
> 0.	Program arguments: /home/umb/LLVM/build/bin/llvm-pdbutil pretty -native -injected-sources -injected-source-content /home/umb/LLVM/llvm/test/tools/llvm-pdbutil/Inputs/InjectedSource.pdb 
>  #0 0x000055b389798f5a llvm::sys::PrintStackTrace(llvm::raw_ostream&) (/home/umb/LLVM/build/bin/llvm-pdbutil+0x2acf5a)
>  #1 0x000055b389796c84 llvm::sys::RunSignalHandlers() (/home/umb/LLVM/build/bin/llvm-pdbutil+0x2aac84)
>  #2 0x000055b389796e05 SignalHandler(int) (/home/umb/LLVM/build/bin/llvm-pdbutil+0x2aae05)
>  #3 0x00007faef95ebdd0 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12dd0)
>  #4 0x00007faef8cb4077 raise /build/glibc-B9XfQf/glibc-2.28/signal/../sysdeps/unix/sysv/linux/raise.c:51:1
>  #5 0x00007faef8c95535 abort /build/glibc-B9XfQf/glibc-2.28/stdlib/abort.c:81:7
>  #6 0x000055b389587813 llvm::Expected<llvm::StringRef>::fatalUncheckedExpected() const (/home/umb/LLVM/build/bin/llvm-pdbutil+0x9b813)
>  #7 0x000055b3896a4eb8 llvm::pdb::(anonymous namespace)::NativeInjectedSource::getFileName() const (/home/umb/LLVM/build/bin/llvm-pdbutil+0x1b8eb8)
>  #8 0x000055b3895bb8ed dumpPretty(llvm::StringRef) (/home/umb/LLVM/build/bin/llvm-pdbutil+0xcf8ed)
>  #9 0x000055b3895501c0 main (/home/umb/LLVM/build/bin/llvm-pdbutil+0x641c0)
> #10 0x00007faef8c9709b __libc_start_main /build/glibc-B9XfQf/glibc-2.28/csu/../csu/libc-start.c:342:3
> #11 0x000055b38956645a _start (/home/umb/LLVM/build/bin/llvm-pdbutil+0x7a45a)
> FileCheck error: '-' is empty.
> FileCheck command line:  /home/umb/LLVM/build/bin/FileCheck /home/umb/LLVM/llvm/test/tools/llvm-pdbutil/injected-sources-native.test
> 
> ```
Basing on the text in `assert()` (I am not familar with the logic of this tool at all), seems just adding a `consumeError` call would be sufficient and correct.


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D64428/new/

https://reviews.llvm.org/D64428