Page MenuHomePhabricator

Fix interaction between -fdiscard-value-names and LLVM Bitcode
AbandonedPublic

Authored by serge-sans-paille on Feb 19 2020, 1:46 PM.

Details

Summary

Without that patch, the test case associated to this patch segfaults in Release mode. This also makes the llvm-test-suite build fails, including in 10.0.0 rc2.

Diff Detail

Unit TestsFailed

TimeTest
1,300 msMLIR.mlir-cpu-runner::bare_ptr_call_conv.mlir
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/mlir-opt /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/mlir-cpu-runner/bare_ptr_call_conv.mlir -convert-loop-to-std -convert-std-to-llvm='use-bare-ptr-memref-call-conv=1' | mlir-cpu-runner -shared-libs=/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/libmlir_runner_utils.so -entry-point-result=void | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/mlir-cpu-runner/bare_ptr_call_conv.mlir
1,310 msMLIR.mlir-cpu-runner::linalg_integration_test.mlir
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/mlir-opt /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/mlir-cpu-runner/linalg_integration_test.mlir -convert-linalg-to-llvm | mlir-cpu-runner -e dot -entry-point-result=f32 -shared-libs=/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/libcblas.so,/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/libcblas_interface.so | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/mlir-cpu-runner/linalg_integration_test.mlir
1,210 msMLIR.mlir-cpu-runner::simple.mlir
Script: -- : 'RUN: at line 1'; mlir-cpu-runner /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/mlir-cpu-runner/simple.mlir | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/mlir-cpu-runner/simple.mlir
1,330 msMLIR.mlir-cpu-runner::unranked_memref.mlir
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/mlir-opt /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/mlir-cpu-runner/unranked_memref.mlir -convert-linalg-to-loops -convert-linalg-to-llvm -convert-std-to-llvm | mlir-cpu-runner -e main -entry-point-result=void -shared-libs=/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/libmlir_runner_utils.so,/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/libcblas.so,/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/libcblas_interface.so | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/mlir-cpu-runner/unranked_memref.mlir
1,160 msMLIR.mlir-cpu-runner::utils.mlir
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/mlir-opt /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/mlir-cpu-runner/utils.mlir -convert-linalg-to-loops -convert-linalg-to-llvm -convert-std-to-llvm | mlir-cpu-runner -e print_0d -entry-point-result=void -shared-libs=/mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/lib/libmlir_runner_utils.so | /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/FileCheck /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/mlir/test/mlir-cpu-runner/utils.mlir --check-prefix=PRINT-0D

Event Timeline

Herald added a project: Restricted Project. · View Herald TranscriptFeb 19 2020, 1:46 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This seems wrong to me. Discarding value names is the default and has been forever in non-assert builds.
Why is this not a bug in w/e is handling the IR?

You mention a test case in the description but I don't see it?

clang/lib/Frontend/CompilerInvocation.cpp
3607

What do you mean by this?

3610

nit: you don't need to test, you can always assign to false

Added test case.

Why is this not a bug in w/e is handling the IR?

It may totally be. Consider this review as a way to start the discussion. The problem, as it appears, is triggered when some value is parsed in non-discard mode (which is the only valid mode when parsing IR (see https://github.com/llvm/llvm-project/blob/master/llvm/lib/AsmParser/LLParser.cpp#L71)) and then the Value is destroyed. Another working patch I've experimented with modifies Value::setName and still runs the function body in discard mode if the value actually has a name. Would you prefer that approach?

Finally, the test case :-)

Other approach to the problem: modify Value::SetNameImpl

Herald added a project: Restricted Project. · View Herald TranscriptFeb 20 2020, 2:12 AM
hans added a reviewer: xur.Feb 20 2020, 5:01 AM

Thanks for looking into this, Serge!

I used your test case to bisect where the crashing started, and ended up at 60d39479221d6bc09060f7816bcd7c54eb286603

Someone else noted that also and filed https://bugs.llvm.org/show_bug.cgi?id=44896

Ron has a fix out at https://reviews.llvm.org/D74878
I don't know which of these two is a better fix.