New test to check if basic block sections produces the right back traces with lldb

[tmsriram]

Summary

This test includes a source that will produce basic blocks and hence sections with -fbasic-block-sections=all.

The test reorders the basic blocks to be dis-contiguous in the address space and checks if the back trace contains the right symbol.

[dblaikie]

I know very little about lldb and lldb's test coverage, so hopefully some other folks can chime in (CC'd @labath and @rupprecht ).

[labath]

I think this looks reasonable. A couple of comments inline.

lldb/test/Shell/Unwind/basic-block-sections.test

5

So, this won't work on arm for instance? How about non-elf x86 platforms?

5

Also, add "lld" to the requires clause.

7

I don't believe call-asm.c is necessary here. It's main purpose is to hide the fact that the main symbol gets mangled differently depending on the platform. But that's not needed if the test is in C.

[DavidSpickett]

lldb/test/Shell/Unwind/basic-block-sections.test

5

Do we also need to require that %clang_host% is actually some version of clang, to have the -fbasic-block-sections option? What if you set LLDB_TEST_COMPILER to be gcc for example.

[labath]

lldb/test/Shell/Unwind/basic-block-sections.test

5

For better or worse, LLDB_TEST_COMPILER only affects the "API" tests. The idea is that they're the ones which are the sort of higher level integration tests between lldb and the compiler. However we're not good at enforcing that distinction (in fact, we're quite bad at it).

[MaskRay]

I think this is like an integration test. Not all Clang supporting -fbasic-block-sections can work. This requires a Clang built at the same commit as LLDB..
Currently this may not require LLD as it does not use any LLD specific features but I don't know the future direction.
There may be some risk that such a test may have slightly larger flakiness.

(D73497 is still sitting in my review queue but it isn't being actively worked on. It seems that basic block sections feature has changed its direction? In that case, "Planning Changes" might be a good action)

[dblaikie]

In D89179#2328141, @MaskRay wrote:

I think this is like an integration test. Not all Clang supporting -fbasic-block-sections can work. This requires a Clang built at the same commit as LLDB..

Not sure I follow - why would this test need to be built with a version matched clang+lldb? I'd expect it needed a clang with at least the functionality being tested, as @labath was mentioning - but I don't /think/ it'd need a version locked clang.

Currently this may not require LLD as it does not use any LLD specific features but I don't know the future direction.
There may be some risk that such a test may have slightly larger flakiness.

What sort of flakiness are you considering here?

(D73497 is still sitting in my review queue but it isn't being actively worked on. It seems that basic block sections feature has changed its direction? In that case, "Planning Changes" might be a good action)

Looks like the last comment on the patch was asking for further review & hasn't been responded to since?

[tmsriram]

Address reviewer comments:

• Remove dependency on call-asm.c
• Remove syms.txt
• add lld dependency

[labath]

I'm still trying to understand what platforms is this supposed to work on -- mainly to see if additional test annotations are needed. What's the story with non-x86 non-elf targets?

In D89179#2328195, @dblaikie wrote:

In D89179#2328141, @MaskRay wrote:

I think this is like an integration test. Not all Clang supporting -fbasic-block-sections can work. This requires a Clang built at the same commit as LLDB..

Not sure I follow - why would this test need to be built with a version matched clang+lldb? I'd expect it needed a clang with at least the functionality being tested, as @labath was mentioning - but I don't /think/ it'd need a version locked clang.

Yeah, that requirement would be pretty surprising, though the way this test is implemented means that it will always run with a version locked clang. If we wanted to explicitly test different versions of the compiler (I don't think it's necessary) this would need to be written as an API test. Just note that the only existing bot configured to run this way is a mac machine, so it will not be of any immediate use anyway (I think).

Currently this may not require LLD as it does not use any LLD specific features but I don't know the future direction.

Isn't --symbol-ordering-file= lld-specific? I don't see it in bfd or gold docs (the latter has --section-ordering-file though).

lldb/test/Shell/Unwind/basic-block-sections.test

14

[tmsriram]

Fix a typo.

[tmsriram]

In D89179#2329922, @labath wrote:

I'm still trying to understand what platforms is this supposed to work on -- mainly to see if additional test annotations are needed. What's the story with non-x86 non-elf targets?

In D89179#2328195, @dblaikie wrote:

In D89179#2328141, @MaskRay wrote:

I think this is like an integration test. Not all Clang supporting -fbasic-block-sections can work. This requires a Clang built at the same commit as LLDB..

Not sure I follow - why would this test need to be built with a version matched clang+lldb? I'd expect it needed a clang with at least the functionality being tested, as @labath was mentioning - but I don't /think/ it'd need a version locked clang.

Yeah, that requirement would be pretty surprising, though the way this test is implemented means that it will always run with a version locked clang. If we wanted to explicitly test different versions of the compiler (I don't think it's necessary) this would need to be written as an API test. Just note that the only existing bot configured to run this way is a mac machine, so it will not be of any immediate use anyway (I think).

I am reading this as rewriting this as an API test would not help us much. If you see benefits with an API test, I am happy to change this or add that too. Thanks!

Currently this may not require LLD as it does not use any LLD specific features but I don't know the future direction.

Isn't --symbol-ordering-file= lld-specific? I don't see it in bfd or gold docs (the latter has --section-ordering-file though).

Yes, only LLD supports symbol-ordering-file, so I explicitly used -fuse-ld=lld. I guess that also means I need the REQUIRES: lld clause.

lldb/test/Shell/Unwind/basic-block-sections.test

5

Not yet been ported to non-ELF or non-x86. Something we plan to do in future.

5

Acknowledged.

14

Sorry!

[labath]

In D89179#2332781, @tmsriram wrote:

In D89179#2329922, @labath wrote:

In D89179#2328195, @dblaikie wrote:

Not sure I follow - why would this test need to be built with a version matched clang+lldb? I'd expect it needed a clang with at least the functionality being tested, as @labath was mentioning - but I don't /think/ it'd need a version locked clang.

Yeah, that requirement would be pretty surprising, though the way this test is implemented means that it will always run with a version locked clang. If we wanted to explicitly test different versions of the compiler (I don't think it's necessary) this would need to be written as an API test. Just note that the only existing bot configured to run this way is a mac machine, so it will not be of any immediate use anyway (I think).

I am reading this as rewriting this as an API test would not help us much. If you see benefits with an API test, I am happy to change this or add that too. Thanks!

I think this is fine.

lldb/test/Shell/Unwind/basic-block-sections.test

5

Ok, in that case, please add UNSUPPORTED: system-darwin, system-windows (one day we should add an system-elf feature...)

[tmsriram]

Address reviewer comments, add UNSUPPORTED: and test one more permutation (the exact reverse) for basic block reordering.

lldb/test/Shell/Unwind/Inputs/basic-block-sections.c

+int k = 1;
+
+int bar() {
+  return 0;
+}
+
+int foo() {
+  return bar();
+}
+
+int main() {
+  // Control flow to create basic block sections.
+  if (k)
+    foo();
+  else
+    bar();
+  return 0;
+}

lldb/test/Shell/Unwind/basic-block-sections.test

+# Test to check if using basic block sections to reorder basic blocks at
+# run-time still produces the right backtraces with lldb.
+
+# UNSUPPORTED: system-darwin, system-windows
+# REQUIRES: target-x86_64
+# REQUIRES: lld
+
+# RUN: %clang_host %p/Inputs/basic-block-sections.c -o %t
+# RUN: %lldb %t -s %s -o exit | FileCheck  %s --check-prefix=DEFAULT
+
+# RUN: %clang_host %p/Inputs/basic-block-sections.c -o %t -fbasic-block-sections=all
+# RUN: %lldb %t -s %s -o exit | FileCheck  %s --check-prefix=BBSECTIONS
+
+# Reorder basic blocks so that main's basic blocks are discontiguous
+# RUN: echo "main.3" > %t.order
+# RUN: echo "bar" >> %t.order
+# RUN: echo "main" >> %t.order
+# RUN: echo "main.2" >> %t.order
+# RUN: echo "foo" >> %t.order
+# RUN: echo "main.1" >> %t.order
+# RUN: %clang_host %p/Inputs/basic-block-sections.c -o %t -fbasic-block-sections=all -fuse-ld=lld -Wl,--symbol-ordering-file,%t.order -Wl,--warn-symbol-ordering -Wl,--fatal-warnings
+# RUN: %lldb %t -s %s -o exit | FileCheck  %s --check-prefix=BBSECTIONS
+
+# Test the reverse permutation too.
+# RUN: echo "main.1" > %t.order
+# RUN: echo "foo" >> %t.order
+# RUN: echo "main.2" >> %t.order
+# RUN: echo "main" >> %t.order
+# RUN: echo "bar" >> %t.order
+# RUN: echo "main.3" >> %t.order
+# RUN: %clang_host %p/Inputs/basic-block-sections.c -o %t -fbasic-block-sections=all -fuse-ld=lld -Wl,--symbol-ordering-file,%t.order -Wl,--warn-symbol-ordering -Wl,--fatal-warnings
+# RUN: %lldb %t -s %s -o exit | FileCheck  %s --check-prefix=BBSECTIONS
+
+breakpoint set -n bar
+# DEFAULT: Breakpoint 1: where = {{.*}}`bar
+# BBSECTIONS: Breakpoint 1: where = {{.*}}`bar
+
+process launch
+# DEFAULT: stop reason = breakpoint 1.1
+# BBSECTIONS: stop reason = breakpoint 1.1
+
+thread backtrace
+# DEFAULT: frame #0: {{.*}}`bar
+# DEFAULT: frame #1: {{.*}}`foo
+# DEFAULT: frame #2: {{.*}}`main +
+
+# BBSECTIONS: frame #0: {{.*}}`bar
+# BBSECTIONS: frame #1: {{.*}}`foo
+# BBSECTIONS: frame #2: {{.*}}`main.1 +