Page MenuHomePhabricator

[lldb] Add reproducer verifier
ClosedPublic

Authored by JDevlieghere on Aug 24 2020, 5:11 PM.

Details

Summary

Add a reproducer verifier that catches:

  • Missing or invalid home directory
  • Missing or invalid working directory
  • Missing or invalid module/symbol paths
  • Missing files from the VFS

Event Timeline

JDevlieghere created this revision.Aug 24 2020, 5:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 24 2020, 5:11 PM
JDevlieghere requested review of this revision.Aug 24 2020, 5:11 PM

This will require a test and maybe some code deduplication in CommandObjectReproducer but I already wanted to put the patch up in case I can't get to that today.

kastiglione added inline comments.
lldb/source/Commands/CommandObjectReproducer.cpp
678

Is this lambda missing an assignment to errors?

JDevlieghere added inline comments.Aug 24 2020, 7:28 PM
lldb/source/Commands/CommandObjectReproducer.cpp
678

Good catch, it is indeed.

  • Add tests
  • Extract common code from CommandObjectReproducerDump and CommandObjectReproducerVerify into GetLoaderFromPathOrCurrent
  • Address code review feedback
JDevlieghere marked an inline comment as done.Aug 25 2020, 10:49 AM
  • Make CHECK-line more specific
teemperor requested changes to this revision.Aug 26 2020, 9:45 AM
teemperor added a subscriber: teemperor.

As discussed offline, I think the warnings here seem also useful when just replaying. I guess we should make it clear that things like missing files are fine when replaying a reproducer (e.g., just pointing that out as a 'note' and not warning/error). Beside that this LGTM.

lldb/source/Commands/CommandObjectReproducer.cpp
146

form -> from

156

tot -> to

603

Is that dumping implemented? It's also kind of surprising that verify dumps the current reproducer instead of verifying it.

This revision now requires changes to proceed.Aug 26 2020, 9:45 AM
  • Address code review feedback
  • Run the verifier when replaying a reproducer
  • Add SBReplayOptions instead of adding another overload
  • Add --reproducer-skip-verify flag
JDevlieghere marked 3 inline comments as done.Aug 26 2020, 11:18 AM
teemperor accepted this revision.Aug 31 2020, 9:57 AM

One nit about naming and another small thing, but otherwise this LGTM.

lldb/include/lldb/Utility/Reproducer.h
242 ↗(On Diff #288051)

I kinda like verify = true a bit better than skip_verification = false. 'skip' feels like a negation, so you end up checks like if (!skip_verification) (= "if we should not not verify" instead of just if (verify). Same below.

But that's just personal preference so I won't mind the name if you feel strongly about it (and it kinda fits the command line argument names)

lldb/source/Utility/Reproducer.cpp
337 ↗(On Diff #288051)

Maybe log the error to llvm::errs? Or is there no symbol-files.yaml when the replay doesn't involve symbol files, so this is expected?

This revision is now accepted and ready to land.Aug 31 2020, 9:57 AM
JDevlieghere marked an inline comment as done.Aug 31 2020, 1:50 PM
JDevlieghere added inline comments.
lldb/source/Utility/Reproducer.cpp
337 ↗(On Diff #288051)

Correct, the symbol files are optional.

This revision was automatically updated to reflect the committed changes.
JDevlieghere marked an inline comment as done.
Herald added a project: Restricted Project. · View Herald TranscriptAug 31 2020, 3:14 PM

Hello. I have an auto-bisecting multi-stage bot that has identified this change as breaking release (without assertions) testing on Fedora 33 x86-64. Can we get a quick fix or revert this change for now?

FAIL: lldb-shell :: Reproducer/Functionalities/TestImageList.test (68782 of 69683)
******************** TEST 'lldb-shell :: Reproducer/Functionalities/TestImageList.test' FAILED ********************
Script:
--
: 'RUN: at line 6';   /tmp/_update_lc/r/bin/clang --target=specify-a-target-or-use-a-_host-substitution --target=x86_64-unknown-linux-gnu -pthread -fmodules-cache-path=/tmp/_update_lc/r/lldb-test-build.noindex/module-cache-clang/lldb-shell /home/dave/ro_s/lp/lldb/test/Shell/Reproducer/Functionalities/Inputs/stepping.c -g -o /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.out
: 'RUN: at line 8';   rm -rf /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.txt
: 'RUN: at line 10';   echo "CAPTURE" >> /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.txt
: 'RUN: at line 11';   /tmp/_update_lc/r/bin/lldb --no-lldbinit -S /tmp/_update_lc/r/tools/lldb/test/Shell/lit-lldb-init -x -b  --capture --capture-path /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.repro     -o 'run'     -o 'image list'     -o 'reproducer generate'     /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.out >> /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.txt 2>&1
: 'RUN: at line 17';   echo "REPLAY" >> /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.txt
: 'RUN: at line 18';   /tmp/_update_lc/r/bin/lldb --no-lldbinit -S /tmp/_update_lc/r/tools/lldb/test/Shell/lit-lldb-init -x -b --replay /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.repro >> /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.txt 2>&1
: 'RUN: at line 20';   cat /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.txt | /tmp/_update_lc/r/bin/FileCheck /home/dave/ro_s/lp/lldb/test/Shell/Reproducer/Functionalities/TestImageList.test
--
Exit Code: 1

Command Output (stderr):
--
clang-12: warning: argument unused during compilation: '-fmodules-cache-path=/tmp/_update_lc/r/lldb-test-build.noindex/module-cache-clang/lldb-shell' [-Wunused-command-line-argument]

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
********************
Failed Tests (1):
  lldb-shell :: Reproducer/Functionalities/TestImageList.test


Testing Time: 125.54s
  Unsupported      : 10774
  Passed           : 58806
  Expectedly Failed:   102
  Failed           :     1
FAILED: CMakeFiles/check-all
teemperor reopened this revision.Sep 1 2020, 3:22 AM

Don't see an obvious fix, so I reverted for Jonas in 7c80f2da812e45bbdfa3c8f9ab24440f8ef3362a

This revision is now accepted and ready to land.Sep 1 2020, 3:22 AM

Hello. I have an auto-bisecting multi-stage bot that has identified this change as breaking release (without assertions) testing on Fedora 33 x86-64. Can we get a quick fix or revert this change for now?

FAIL: lldb-shell :: Reproducer/Functionalities/TestImageList.test (68782 of 69683)
******************** TEST 'lldb-shell :: Reproducer/Functionalities/TestImageList.test' FAILED ********************
Script:
--
: 'RUN: at line 6';   /tmp/_update_lc/r/bin/clang --target=specify-a-target-or-use-a-_host-substitution --target=x86_64-unknown-linux-gnu -pthread -fmodules-cache-path=/tmp/_update_lc/r/lldb-test-build.noindex/module-cache-clang/lldb-shell /home/dave/ro_s/lp/lldb/test/Shell/Reproducer/Functionalities/Inputs/stepping.c -g -o /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.out
: 'RUN: at line 8';   rm -rf /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.txt
: 'RUN: at line 10';   echo "CAPTURE" >> /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.txt
: 'RUN: at line 11';   /tmp/_update_lc/r/bin/lldb --no-lldbinit -S /tmp/_update_lc/r/tools/lldb/test/Shell/lit-lldb-init -x -b  --capture --capture-path /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.repro     -o 'run'     -o 'image list'     -o 'reproducer generate'     /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.out >> /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.txt 2>&1
: 'RUN: at line 17';   echo "REPLAY" >> /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.txt
: 'RUN: at line 18';   /tmp/_update_lc/r/bin/lldb --no-lldbinit -S /tmp/_update_lc/r/tools/lldb/test/Shell/lit-lldb-init -x -b --replay /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.repro >> /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.txt 2>&1
: 'RUN: at line 20';   cat /tmp/_update_lc/r/tools/lldb/test/Reproducer/Functionalities/Output/TestImageList.test.tmp.txt | /tmp/_update_lc/r/bin/FileCheck /home/dave/ro_s/lp/lldb/test/Shell/Reproducer/Functionalities/TestImageList.test
--
Exit Code: 1

Command Output (stderr):
--
clang-12: warning: argument unused during compilation: '-fmodules-cache-path=/tmp/_update_lc/r/lldb-test-build.noindex/module-cache-clang/lldb-shell' [-Wunused-command-line-argument]

--

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90..
********************
Failed Tests (1):
  lldb-shell :: Reproducer/Functionalities/TestImageList.test


Testing Time: 125.54s
  Unsupported      : 10774
  Passed           : 58806
  Expectedly Failed:   102
  Failed           :     1
FAILED: CMakeFiles/check-all

Hey Dave, I cannot repro this, neither on macOS nor Linux when building in release without assertions. Could you rerun the test manually and copy the output of the different commands? The lit output is really abysmal :(

Line 18 fails: %lldb -x -b --replay %t.repro >> %t.txt 2>&1

error: reproducer replay failed:
warning: home directory '/home/dave' not in VFS

I tried rebuilding in my home directory as opposed to /tmp and the error went away. Did you try building outside of your home directory?

And just to be clear, the source directory in my setup is in the home directory. My cron job / "bot" build just builds in /tmp.

This revision was automatically updated to reflect the committed changes.