[DebugInfo] Removal of DIFortranArrayType and DIFortranSubrange #10

alokkrsharma · 2021-01-07T06:46:23Z

These extensions are no more required after merge of below PR.
#8

bryanpkc

The DIBasicType constructors on lines 786 to 791 in llvm/include/llvm/IR/DebugInfoMetadata.h were added in commit flang-compiler/llvm@80698caba789e, along with DIFortranArrayType. Are they still necessary or could they also be removed?

DwarfCompileUnit::constructDieLocation, DwarfCompileUnit::constructDieLocationAddExpr, and a bunch of changes to DwarfCompileUnit::constructVariableDIEImpl were also added in commit flang-compiler/llvm@80698caba789e. Should these changes be reverted?

llvm/include/llvm/Bitcode/LLVMBitCodes.h

alokkrsharma · 2021-01-17T15:16:04Z

The DIBasicType constructors on lines 786 to 791 in llvm/include/llvm/IR/DebugInfoMetadata.h were added in commit flang-compiler/llvm@80698ca, along with DIFortranArrayType. Are they still necessary or could they also be removed?

DwarfCompileUnit::constructDieLocation, DwarfCompileUnit::constructDieLocationAddExpr, and a bunch of changes to DwarfCompileUnit::constructVariableDIEImpl were also added in commit flang-compiler/llvm@80698ca. Should these changes be reverted?

Thanks for pointing this out. These comments are addressed.

When `Target::GetEntryPointAddress()` calls `exe_module->GetObjectFile()->GetEntryPointAddress()`, and the returned `entry_addr` is valid, it can immediately be returned. However, just before that, an `llvm::Error` value has been setup, but in this case it is not consumed before returning, like is done further below in the function. In https://bugs.freebsd.org/248745 we got a bug report for this, where a very simple test case aborts and dumps core: ``` * thread #1, name = 'testcase', stop reason = breakpoint 1.1 frame #0: 0x00000000002018d4 testcase`main(argc=1, argv=0x00007fffffffea18) at testcase.c:3:5 1 int main(int argc, char *argv[]) 2 { -> 3 return 0; 4 } (lldb) p argc Program aborted due to an unhandled Error: Error value was Success. (Note: Success values must still be checked prior to being destroyed). Thread 1 received signal SIGABRT, Aborted. thr_kill () at thr_kill.S:3 3 thr_kill.S: No such file or directory. (gdb) bt #0 thr_kill () at thr_kill.S:3 #1 0x00000008049a0004 in __raise (s=6) at /usr/src/lib/libc/gen/raise.c:52 #2 0x0000000804916229 in abort () at /usr/src/lib/libc/stdlib/abort.c:67 #3 0x000000000451b5f5 in fatalUncheckedError () at /usr/src/contrib/llvm-project/llvm/lib/Support/Error.cpp:112 #4 0x00000000019cf008 in GetEntryPointAddress () at /usr/src/contrib/llvm-project/llvm/include/llvm/Support/Error.h:267 #5 0x0000000001bccbd8 in ConstructorSetup () at /usr/src/contrib/llvm-project/lldb/source/Target/ThreadPlanCallFunction.cpp:67 #6 0x0000000001bcd2c0 in ThreadPlanCallFunction () at /usr/src/contrib/llvm-project/lldb/source/Target/ThreadPlanCallFunction.cpp:114 #7 0x00000000020076d4 in InferiorCallMmap () at /usr/src/contrib/llvm-project/lldb/source/Plugins/Process/Utility/InferiorCallPOSIX.cpp:97 #8 0x0000000001f4be33 in DoAllocateMemory () at /usr/src/contrib/llvm-project/lldb/source/Plugins/Process/FreeBSD/ProcessFreeBSD.cpp:604 #9 0x0000000001fe51b9 in AllocatePage () at /usr/src/contrib/llvm-project/lldb/source/Target/Memory.cpp:347 #10 0x0000000001fe5385 in AllocateMemory () at /usr/src/contrib/llvm-project/lldb/source/Target/Memory.cpp:383 #11 0x0000000001974da2 in AllocateMemory () at /usr/src/contrib/llvm-project/lldb/source/Target/Process.cpp:2301 #12 CanJIT () at /usr/src/contrib/llvm-project/lldb/source/Target/Process.cpp:2331 #13 0x0000000001a1bf3d in Evaluate () at /usr/src/contrib/llvm-project/lldb/source/Expression/UserExpression.cpp:190 #14 0x00000000019ce7a2 in EvaluateExpression () at /usr/src/contrib/llvm-project/lldb/source/Target/Target.cpp:2372 #15 0x0000000001ad784c in EvaluateExpression () at /usr/src/contrib/llvm-project/lldb/source/Commands/CommandObjectExpression.cpp:414 #16 0x0000000001ad86ae in DoExecute () at /usr/src/contrib/llvm-project/lldb/source/Commands/CommandObjectExpression.cpp:646 #17 0x0000000001a5e3ed in Execute () at /usr/src/contrib/llvm-project/lldb/source/Interpreter/CommandObject.cpp:1003 #18 0x0000000001a6c4a3 in HandleCommand () at /usr/src/contrib/llvm-project/lldb/source/Interpreter/CommandInterpreter.cpp:1762 #19 0x0000000001a6f98c in IOHandlerInputComplete () at /usr/src/contrib/llvm-project/lldb/source/Interpreter/CommandInterpreter.cpp:2760 #20 0x0000000001a90b08 in Run () at /usr/src/contrib/llvm-project/lldb/source/Core/IOHandler.cpp:548 #21 0x00000000019a6c6a in ExecuteIOHandlers () at /usr/src/contrib/llvm-project/lldb/source/Core/Debugger.cpp:903 #22 0x0000000001a70337 in RunCommandInterpreter () at /usr/src/contrib/llvm-project/lldb/source/Interpreter/CommandInterpreter.cpp:2946 #23 0x0000000001d9d812 in RunCommandInterpreter () at /usr/src/contrib/llvm-project/lldb/source/API/SBDebugger.cpp:1169 #24 0x0000000001918be8 in MainLoop () at /usr/src/contrib/llvm-project/lldb/tools/driver/Driver.cpp:675 #25 0x000000000191a114 in main () at /usr/src/contrib/llvm-project/lldb/tools/driver/Driver.cpp:890``` Fix the incorrect error catch by only instantiating an `Error` object if it is necessary. Reviewed By: JDevlieghere Differential Revision: https://reviews.llvm.org/D86355 (cherry picked from commit 1ce07cd)

alokkrsharma · 2021-01-29T11:30:48Z

Hi @bryanpkc , I have incorporated all your comments except one related to renumbering of METADATA_COMMON_BLOCK and METADATA_GENERIC_SUBRANGE due to fear of breaking compatibility. compiling (*.bc generated with older versions or by opensource llvm) with newer (numbering) compiler.
Please let me know your opinion ?

bryanpkc

Thanks, and sorry for the late response.

alokkrsharma · 2021-02-16T15:50:50Z

Hi @kiranchandramohan, @shivaramaarao do you have any comment on this ?

kiranchandramohan · 2021-02-21T12:20:20Z

@alokkrsharma Can you rebase and create a similar patch for llvm-11 as well?

kiranchandramohan · 2021-02-22T07:48:51Z

@alokkrsharma can you try again?

michalpasztamobica · 2021-02-22T07:57:08Z

@alokkrsharma , something went wrong with your rebase, perhaps you forgot to pull from classic-flang-llvm-project? You need to make sure you're on top of this PR and in particular this commit 5cab966 to get rid of the error you're seeing now.

kiranchandramohan · 2021-02-22T08:01:17Z

@michalpasztamobica That was for release_11x only I think. I submitted the following yesterday for release_100.
b93cbc1

michalpasztamobica · 2021-02-22T08:07:41Z

@kiranchandramohan , you are right. Alok's rebase was correct, but the fix was only for release_11x. Now CI is passing with your change also for release_100, so another rebase will fix this PR.

These extensions are no more required after merge of below PR. flang-compiler#8

alokkrsharma · 2021-02-26T07:43:40Z

Hi @kiranchandramohan and @shivaramaarao is this PR good to be merged ?

kiranchandramohan

LGTM.

This patch implemented TTI.IntImmCost() properly. Each BPF insn has 32bit immediate space, so for any immediate which can be represented as 32bit signed int, the cost is technically free. If an int cannot be presented as a 32bit signed int, a ld_imm64 instruction is needed and a TCC_Basic is returned. This change is motivated when we observed that several bpf selftests failed with latest llvm trunk, e.g., flang-compiler#10/16 strobemeta.o:FAIL flang-compiler#10/17 strobemeta_nounroll1.o:FAIL flang-compiler#10/18 strobemeta_nounroll2.o:FAIL flang-compiler#10/19 strobemeta_subprogs.o:FAIL flang-compiler#96 snprintf_btf:FAIL The reason of the failure is due to that SpeculateAroundPHIsPass did aggressive transformation which alters control flow for which currently verifer cannot handle well. In llvm12, SpeculateAroundPHIsPass is not called. SpeculateAroundPHIsPass relied on TTI.getIntImmCost() and TTI.getIntImmCostInst() for profitability analysis. This patch implemented TTI.getIntImmCost() properly for BPF backend which also prevented transformation which caused the above test failures. Differential Revision: https://reviews.llvm.org/D96448 (cherry picked from commit a260ae7)

This patch implemented TTI.IntImmCost() properly. Each BPF insn has 32bit immediate space, so for any immediate which can be represented as 32bit signed int, the cost is technically free. If an int cannot be presented as a 32bit signed int, a ld_imm64 instruction is needed and a TCC_Basic is returned. This change is motivated when we observed that several bpf selftests failed with latest llvm trunk, e.g., #10/16 strobemeta.o:FAIL #10/17 strobemeta_nounroll1.o:FAIL #10/18 strobemeta_nounroll2.o:FAIL #10/19 strobemeta_subprogs.o:FAIL #96 snprintf_btf:FAIL The reason of the failure is due to that SpeculateAroundPHIsPass did aggressive transformation which alters control flow for which currently verifer cannot handle well. In llvm12, SpeculateAroundPHIsPass is not called. SpeculateAroundPHIsPass relied on TTI.getIntImmCost() and TTI.getIntImmCostInst() for profitability analysis. This patch implemented TTI.getIntImmCost() properly for BPF backend which also prevented transformation which caused the above test failures. Differential Revision: https://reviews.llvm.org/D96448 (cherry picked from commit a260ae7)

We experienced some deadlocks when we used multiple threads for logging using `scan-builds` intercept-build tool when we used multiple threads by e.g. logging `make -j16` ``` (gdb) bt #0 0x00007f2bb3aff110 in __lll_lock_wait () from /lib/x86_64-linux-gnu/libpthread.so.0 flang-compiler#1 0x00007f2bb3af70a3 in pthread_mutex_lock () from /lib/x86_64-linux-gnu/libpthread.so.0 flang-compiler#2 0x00007f2bb3d152e4 in ?? () flang-compiler#3 0x00007ffcc5f0cc80 in ?? () flang-compiler#4 0x00007f2bb3d2bf5b in ?? () from /lib64/ld-linux-x86-64.so.2 flang-compiler#5 0x00007f2bb3b5da27 in ?? () from /lib/x86_64-linux-gnu/libc.so.6 flang-compiler#6 0x00007f2bb3b5dbe0 in exit () from /lib/x86_64-linux-gnu/libc.so.6 flang-compiler#7 0x00007f2bb3d144ee in ?? () flang-compiler#8 0x746e692f706d742f in ?? () flang-compiler#9 0x692d747065637265 in ?? () flang-compiler#10 0x2f653631326b3034 in ?? () flang-compiler#11 0x646d632e35353532 in ?? () flang-compiler#12 0x0000000000000000 in ?? () ``` I think the gcc's exit call caused the injected `libear.so` to be unloaded by the `ld`, which in turn called the `void on_unload() __attribute__((destructor))`. That tried to acquire an already locked mutex which was left locked in the `bear_report_call()` call, that probably encountered some error and returned early when it forgot to unlock the mutex. All of these are speculation since from the backtrace I could not verify if frames 2 and 3 are in fact corresponding to the `libear.so` module. But I think it's a fairly safe bet. So, hereby I'm releasing the held mutex on *all paths*, even if some failure happens. PS: I would use lock_guards, but it's C. Reviewed-by: NoQ Differential Revision: https://reviews.llvm.org/D118439 (cherry picked from commit d919d02)

alokkrsharma mentioned this pull request Jan 7, 2021

[DebugInfo] Backporting multiple upstream commits supporting fortran array debuggability #8

Merged

bryanpkc requested changes Jan 15, 2021

View reviewed changes

llvm/include/llvm/Bitcode/LLVMBitCodes.h Show resolved Hide resolved

bryanpkc approved these changes Jan 29, 2021

View reviewed changes

bryanpkc requested review from kiranchandramohan and shivaramaarao January 29, 2021 20:31

alokkrsharma force-pushed the DIFortranArrayCleanup branch from fa2ecaf to 013e53c Compare February 21, 2021 13:49

alokkrsharma added 2 commits February 23, 2021 16:19

[DebugInfo] Removal of DIFortranArrayType and DIFortranSubrange

65860df

These extensions are no more required after merge of below PR. flang-compiler#8

Modification to incorporate comments from @bryanpkc .

6044ea2

alokkrsharma force-pushed the DIFortranArrayCleanup branch from 013e53c to 6044ea2 Compare February 23, 2021 10:50

kiranchandramohan approved these changes Mar 14, 2021

View reviewed changes

kiranchandramohan merged commit f75278c into flang-compiler:release_100 Mar 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[DebugInfo] Removal of DIFortranArrayType and DIFortranSubrange #10

[DebugInfo] Removal of DIFortranArrayType and DIFortranSubrange #10

Uh oh!

alokkrsharma commented Jan 7, 2021

Uh oh!

bryanpkc left a comment

Uh oh!

Uh oh!

alokkrsharma commented Jan 17, 2021

Uh oh!

alokkrsharma commented Jan 29, 2021

Uh oh!

bryanpkc left a comment

Uh oh!

alokkrsharma commented Feb 16, 2021

Uh oh!

kiranchandramohan commented Feb 21, 2021

Uh oh!

kiranchandramohan commented Feb 22, 2021

Uh oh!

michalpasztamobica commented Feb 22, 2021

Uh oh!

kiranchandramohan commented Feb 22, 2021

Uh oh!

michalpasztamobica commented Feb 22, 2021

Uh oh!

alokkrsharma commented Feb 26, 2021

Uh oh!

kiranchandramohan left a comment

Uh oh!

Uh oh!

[DebugInfo] Removal of DIFortranArrayType and DIFortranSubrange #10

[DebugInfo] Removal of DIFortranArrayType and DIFortranSubrange #10

Uh oh!

Conversation

alokkrsharma commented Jan 7, 2021

Uh oh!

bryanpkc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alokkrsharma commented Jan 17, 2021

Uh oh!

alokkrsharma commented Jan 29, 2021

Uh oh!

bryanpkc left a comment

Choose a reason for hiding this comment

Uh oh!

alokkrsharma commented Feb 16, 2021

Uh oh!

kiranchandramohan commented Feb 21, 2021

Uh oh!

kiranchandramohan commented Feb 22, 2021

Uh oh!

michalpasztamobica commented Feb 22, 2021

Uh oh!

kiranchandramohan commented Feb 22, 2021

Uh oh!

michalpasztamobica commented Feb 22, 2021

Uh oh!

alokkrsharma commented Feb 26, 2021

Uh oh!

kiranchandramohan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!