This change to dsymutil allows for, when dsymutil crashes, the user to collect and submit
the object files and a map file so the crash can be reproduced
|3,590 ms||x64 debian > libarcher.races::lock-unrelated.c|
Script: -- : 'RUN: at line 13'; /var/lib/buildkite-agent/builds/llvm-project/build/./bin/clang -fopenmp -pthread -fno-experimental-isel -g -O1 -fsanitize=thread -I /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests -I /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/runtime/src -L /var/lib/buildkite-agent/builds/llvm-project/build/lib -Wl,-rpath,/var/lib/buildkite-agent/builds/llvm-project/build/lib /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/lock-unrelated.c -o /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp -latomic && env TSAN_OPTIONS='ignore_noninstrumented_modules=0:ignore_noninstrumented_modules=1' /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/deflake.bash /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp 2>&1 | tee /var/lib/buildkite-agent/builds/llvm-project/build/projects/openmp/tools/archer/tests/races/Output/lock-unrelated.c.tmp.log | /var/lib/buildkite-agent/builds/llvm-project/build/./bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/openmp/tools/archer/tests/races/lock-unrelated.c
How is collect-crash-inputs different from gan-reproducer? It seems like they're essentially doing the same thing. Can we implement the crash recovery in terms of llvm::dsymutil::Reproducer?
If we split this into a separate class, this can remain here, and have the crash variant print the "PLEASE ATTACH THE FILES IN THE FOLLOWING DIRECTORY TO THE BUG REPORT" in the generate crash class.
I think we probably want a separate class that corresponds to GenerateForCrash.
This seems like it should be part of the reproducers class.
This should be consistent with the header. It's fine (and common) to use the same name for the argument and member variable.
LLVM generally does not prefer braced initializer lists (https://llvm.org/docs/CodingStandards.html#do-not-use-braced-initializer-lists-to-call-a-constructor)
You should use LLVM_FALLTHROUGH, otherwise this won't work if the host compiler is not clang.
No braces around single line statements (https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements)
LLVM doesn't use exceptions (and generally builds without them).
If we're going to store the root, this might as well take a std::string too and move it into Root.
Rather than returning a small string with arbitrary lengths, the common pattern in llvm is to take let the caller decide on the storage type and pass in a llvm::SmallVectorImpl<char>& that the function then populates. This also avoids any potential copies.
I don't think auto is a good fit here (https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable)
Unrelated whitespace change?
Why is this needed?
I rolled up all the comments save one where I just have an outstanding question and then I'll roll that up as well
Got it. Thanks for passing this along. I really should have reviewed it before this submission but I certainly will before the next
yeah. Sorry it snuck in.
So my understanding is that when dsymutil crashes we want the mapping.yaml file, the binary and associated .o file (assuming its compiled with -g). The latter maybe for some workflows? Reusing this gets us the object file but at the cost of parsing the debug map. So maybe I should just copy the binary in? I can do that or just parse enough to recreate the object files. Or maybe I am misunderstanding this entirely?
We need the whole directory, so might as well ask the reporter to include it as a whole.
******************** PLEASE ATTACH THE FOLLOWING DIRECTORY TO THE BUG REPORT:
The nullptr here is redundant.
Don't use auto here, it's not obvious from context what the type is.
Let's be consistent with all the surrounding code and the style guide.
I actually don't think this is going to work. You're creating the reproducer after we've crashed and you're not telling dsymutil to use the VFS from that Reproducer instance. It's the VFS that records all access and is what will allow us to copy all the files in place if we decide we need to keep the reproducer.
The way I would imagine this to work is:
4a. If we crash, we generate the reproducer and print the dsymutil invocation to replay the reproducer.
Does that make sense?
This is not yet complete. I missed step 4a. "4a. If we crash, we generate the reproducer and print the dsymutil invocation to replay the reproducer." It currently doesn't print the command line
Why is this a fallthrough? It seems like now, if we fail to create the ReproducerGenerateForCrash instance, we'll try creating a regular ReproducerGenerate and only then print the error message? I think we should just handle the EC immediately.
Do we still need the ability to specify the mode?
Why do we need a move ctor?
I'm not sure if this is really something we want, with any real-world project the number of object files can quickly become overwhelming. Even if we do, this should probably be a property of the ReproducerGenerate and not be specific for the crash one.
I have a few outstanding questions
I don't think so. But maybe my read is incorrect? The unique_ptr would still get created and thus the nullptr check wouldn't pass and so the ec check below would still do its thing.
thats there for the classof dyn_cast business.
So the answer is found around line 734 in dsymutil.cpp. Since we pass a unique_ptr into the main function for the Reproducer if the user selects -gen-reproducer and the program then crashes we need to move the ReproducerGenerate object into the GenerateForCrash so we get the expected messages.
Since we are passing the Reproducer unique_ptr into the main function and then, in the case where no -gen-reproducer is passed and the program didn't crash we need to make sure we don't dump the files. That was the intent here. But I can (and did) move it to the parent class. So a question that arises from your comment...should we clean up the dumped files on a crash back to just the binary plus mapping file?