This is an archive of the discontinued LLVM Phabricator instance.

Differential D77780

Remove -implicit-check-not=foo from X86/disassemble-functions.test
AbandonedPublic

Authored by xiangzhangllvm on Apr 8 2020, 11:35 PM.

Details

Reviewers
jhenderson
craig.topper
LuoYuanke
annita.zhang
pengfei
MaskRay
Summary

Some people reflect that the test will failed randomly.
I didn't reproduce the error in my local, but I read the llvm-objdump source code, I think I find out the reason:

1, In fact this test-self is not good, (should not use -implicit-check*-not*='foo'). //objdump tool will show foo here.

2, This simple-executable-x86_64.yaml file will generate un-relocatable obj file by tool yaml2obj.

3,For un-relocatable obj file, llvm-objdump will find a target address(related with a symbol) at follow ways:

3-1: collect all symbol with its section.

3-2: sort the sections with the start address order.

3-3: find which section the target address in.

3-4: find the right symbol in that section.

4, the error for this test is that, there are several sections have the some start address 0x00000000. So

it random go to the wrong section to find the target address's symbol. In other word, it some times don't know

which section is right when there more than one sections have the same target address the instruction needed.

ymbol table '.symtab' contains 12 entries:
Num: Value Size Type Bind Vis Ndx Name
0: 0000000000000000 0 NOTYPE LOCAL DEFAULT UND
1: 0000000000000000 0 SECTION LOCAL DEFAULT 1 .text
2: 0000000000000010 0 SECTION LOCAL DEFAULT 2 .anothertext
3: 0000000000000050 0 SECTION LOCAL DEFAULT 3 .eh_frame
4: 00000000000000a8 0 SECTION LOCAL DEFAULT 4 .data
5: 0000000000000000 0 SECTION LOCAL DEFAULT 5 .comment
6: 0000000000000000 0 FILE LOCAL DEFAULT UND /tmp/a.c
7: 0000000000000000 0 FILE LOCAL DEFAULT UND
8: 0000000000000045 0 OBJECT GLOBAL DEFAULT 2 somedata
9: 0000000000000010 63 FUNC GLOBAL DEFAULT 2 main
10: 0000000000000000 13 FUNC GLOBAL DEFAULT 1 foo
11: 00000000000000a8 4 OBJECT GLOBAL DEFAULT 4 a

Diff Detail

Unit TestsFailed

TimeTest
420 msLLVM.Transforms/Attributor::Unknown Unit Message ("")
290 msLLVM.Transforms/Attributor::Unknown Unit Message ("")
90 msLLVM.Transforms/Attributor/ArgumentPromotion::Unknown Unit Message ("")

Event Timeline

xiangzhangllvm created this revision.Apr 8 2020, 11:35 PM
Herald added a reviewer: jhenderson. · View Herald TranscriptApr 8 2020, 11:35 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: llvm-commits, rupprecht, MaskRay. · View Herald Transcript
xiangzhangllvm edited the summary of this revision. (Show Details)Apr 8 2020, 11:43 PM
xiangzhangllvm added reviewers: craig.topper, LuoYuanke, annita.zhang, pengfei.
xiangzhangllvm added a comment.Apr 8 2020, 11:46 PM

@jhenderson I find the test is changed by you. Why you do not want to check "foo" here. Thank you.

xiangzhangllvm added a reviewer: MaskRay.Apr 8 2020, 11:48 PM
xiangzhangllvm added a comment.Apr 8 2020, 11:57 PM
 5 Disassembly of section .text:
 6
 7 0000000000000000 foo:
 8        0: 55                            pushq   %rbp
 9        1: 48 89 e5                      movq    %rsp, %rbp
10        4: 8b 04 25 a8 00 00 00          movl    168, %eax
11        b: 5d                            popq    %rbp
12        c: c3                            retq
13        d: 0f 1f 00                      nopl    (%rax)
14
15 Disassembly of section .anothertext:
16
17 0000000000000010 main:
18       10: 55                            pushq   %rbp
19       11: 48 89 e5                      movq    %rsp, %rbp
20       14: 48 83 ec 20                   subq    $32, %rsp
21       18: 48 8d 04 25 a8 00 00 00       leaq    168, %rax
22       20: c7 45 fc 00 00 00 00          movl    $0, -4(%rbp)
23       27: 48 89 45 f0                   movq    %rax, -16(%rbp)
24       2b: 48 8b 45 f0                   movq    -16(%rbp), %rax
25       2f: 8b 08                         movl    (%rax), %ecx
26       31: 89 4d ec                      movl    %ecx, -20(%rbp)
27       34: e8 c7 ff ff ff                callq   -57 </tmp/a.c>        // =call 0x00000000,   someone reflected there will be <foo> // objdump will dump "foo" here
28       39: 8b 4d ec                      movl    -20(%rbp), %ecx
29       3c: 01 c1                         addl    %eax, %ecx
30       3e: 89 c8                         movl    %ecx, %eax
31       40: 48 83 c4 20                   addq    $32, %rsp
32       44: 5d                            popq    %rbp
MaskRay added a comment.Apr 9 2020, 12:10 AM

Can you double check the non-determinism after D77640?

MaskRay added a subscriber: aqjune.Apr 9 2020, 12:12 AM
Harbormaster failed remote builds in B52459: Diff 256197!Apr 9 2020, 12:30 AM
Higuoxing added a subscriber: Higuoxing.Apr 9 2020, 12:35 AM

I guess we want to test dumping different symbols here. If we specify --disassemble-symbols=main, we don't want llvm-objdump to dump the content of foo here. So, we should ensure llvm-objdump will not print foo. At least, I think we should not remove --implicit-check-not="<foo>:", but improve the robustness of checking?

jhenderson requested changes to this revision.Apr 9 2020, 1:22 AM
In D77780#1971300, @MaskRay wrote:

Can you double check the non-determinism after D77640?

As @MaskRay mentions, I just fixed some non-determinism in how llvm-objdump disassembles things when there are two sections with the same address. This should fix the occasional failures you have been seeing. I also have a follow-up change in progress (not quite ready for review yet, but likely either today or Tuesday when I'm working next), which will make symbols more likely to be found than they were before.

1, In fact this test-self is not good, (should not use -implicit-check*-not*='foo'). //objdump tool will show foo here.

This test should check that the foo function is NOT disassembled, because it was not requested by --disassemble-symbols. That is why the --implicit-check-not is there. Please do not delete it.

I also noticed that the --implicit-check-not pattern you posted here is not what is in the test now. See commit rG27f303924e0b32e22820fa38cb659e9694954784 which changed that and would also prevent this test failing I believe for you, even without the mentioned fix.

What might need doing is fixing the YAML input to not have two non-empty sections at the same address, as this isn't valid.

Aside: there are a few legacy bits in this and related tests referring to "--disassemble-functions" which are incorrect, since the switch was renamed. We should clean those up at some point.

This revision now requires changes to proceed.Apr 9 2020, 1:22 AM
xiangzhangllvm added a comment.Apr 9 2020, 2:25 AM
In D77780#1971300, @MaskRay wrote:

Can you double check the non-determinism after D77640?

I never reproduce the tests in my local. I was told by other people who happen to see the error. I'll check the logic of D77640. Thanks!

xiangzhangllvm added a comment.Apr 9 2020, 2:28 AM
In D77780#1971343, @Higuoxing wrote:

I guess we want to test dumping different symbols here. If we specify --disassemble-symbols=main, we don't want llvm-objdump to dump the content of foo here. So, we should ensure llvm-objdump will not print foo. At least, I think we should not remove --implicit-check-not="<foo>:", but improve the robustness of checking?

--disassemble-symbols=main should just show main's function. From the finding target address in llvm-objdump code seem do not care about "--disassemble-symbols".

xiangzhangllvm added a comment.Apr 9 2020, 2:34 AM

OK, I'll recheck the --disassemble-symbols, that is really strange for me the "--disassemble-symbols=main" will mask the other symbols in "main".

jhenderson added a comment.Apr 9 2020, 2:56 AM
In D77780#1971509, @xiangzhangllvm wrote:

OK, I'll recheck the --disassemble-symbols, that is really strange for me the "--disassemble-symbols=main" will mask the other symbols in "main".

I'm not sure what you mean by "mask the other symbols", but I suspect you are misunderstanding the purpose of the test. On my machine with a recent build of llvm-objdump (post the recent fix I made), the output of this test is:

C:\llvm\build\test\tools\llvm-objdump\X86\Output\disassemble-functions.test.tmp.out:    file format elf64-x86-64


Disassembly of section .anothertext:

0000000000000010 <main>:
      10: 55                            pushq   %rbp
      11: 48 89 e5                      movq    %rsp, %rbp
      14: 48 83 ec 20                   subq    $32, %rsp
      18: 48 8d 04 25 a8 00 00 00       leaq    168, %rax
      20: c7 45 fc 00 00 00 00          movl    $0, -4(%rbp)
      27: 48 89 45 f0                   movq    %rax, -16(%rbp)
      2b: 48 8b 45 f0                   movq    -16(%rbp), %rax
      2f: 8b 08                         movl    (%rax), %ecx
      31: 89 4d ec                      movl    %ecx, -20(%rbp)
      34: e8 c7 ff ff ff                callq   0x0 </tmp/a.c>
      39: 8b 4d ec                      movl    -20(%rbp), %ecx
      3c: 01 c1                         addl    %eax, %ecx
      3e: 89 c8                         movl    %ecx, %eax
      40: 48 83 c4 20                   addq    $32, %rsp
      44: 5d                            popq    %rbp

Without the --disassemble-symbols=main, the output is:

Disassembly of section .text:

0000000000000000 <foo>:
       0: 55                            pushq   %rbp
       1: 48 89 e5                      movq    %rsp, %rbp
       4: 8b 04 25 a8 00 00 00          movl    168, %eax
       b: 5d                            popq    %rbp
       c: c3                            retq
       d: 0f 1f 00                      nopl    (%rax)

Disassembly of section .anothertext:

0000000000000010 <main>:
      10: 55                            pushq   %rbp
      11: 48 89 e5                      movq    %rsp, %rbp
      14: 48 83 ec 20                   subq    $32, %rsp
      18: 48 8d 04 25 a8 00 00 00       leaq    168, %rax
      20: c7 45 fc 00 00 00 00          movl    $0, -4(%rbp)
      27: 48 89 45 f0                   movq    %rax, -16(%rbp)
      2b: 48 8b 45 f0                   movq    -16(%rbp), %rax
      2f: 8b 08                         movl    (%rax), %ecx
      31: 89 4d ec                      movl    %ecx, -20(%rbp)
      34: e8 c7 ff ff ff                callq   0x0 </tmp/a.c>
      39: 8b 4d ec                      movl    -20(%rbp), %ecx
      3c: 01 c1                         addl    %eax, %ecx
      3e: 89 c8                         movl    %ecx, %eax
      40: 48 83 c4 20                   addq    $32, %rsp
      44: 5d                            popq    %rbp

0000000000000045 <somedata>:
      45: 74 65 73 74 20 73 74 72         test str
      4d: 00 c3                           ..

By adding --implicit-check-not=<foo>: to the FileCheck call, we show that the <foo> function is not disassembled, i.e. it is not printed before or after the disassembly of <main>. If you remove it, you will stop the test trying to test what it is for. For example, if llvm-objdump started disassembling the <foo> function, the test would no longer fail.

The current --implicit-check-not pattern does not prevent other appearances of foo in the output, such as because of the callq within the main function (note the improved implicit-check-not pattern I mentioned from commit rG27f303924e0b32e22820fa38cb659e9694954784). However, the current (stable) output of llvm-objdump does not cause foo to appear in main anyway, so even then, I don't think this is an issue. With my in-progress change, you will see <foo> appear as the target of callq:

0000000000000010 <main>:
      10: 55                            pushq   %rbp
      11: 48 89 e5                      movq    %rsp, %rbp
      14: 48 83 ec 20                   subq    $32, %rsp
      18: 48 8d 04 25 a8 00 00 00       leaq    168, %rax
      20: c7 45 fc 00 00 00 00          movl    $0, -4(%rbp)
      27: 48 89 45 f0                   movq    %rax, -16(%rbp)
      2b: 48 8b 45 f0                   movq    -16(%rbp), %rax
      2f: 8b 08                         movl    (%rax), %ecx
      31: 89 4d ec                      movl    %ecx, -20(%rbp)
      34: e8 c7 ff ff ff                callq   0x0 <foo>
      39: 8b 4d ec                      movl    -20(%rbp), %ecx
      3c: 01 c1                         addl    %eax, %ecx
      3e: 89 c8                         movl    %ecx, %eax
      40: 48 83 c4 20                   addq    $32, %rsp
      44: 5d                            popq    %rbp

but this test will still pass with the existing --implicit-check-not pattern, because there is no ':' after <foo> in the call target.

xiangzhangllvm added a comment.EditedApr 9 2020, 5:51 PM

All things clear:
The test have been changed from --implicit-check-not=foo to --implicit-check-not=<foo>: last month.
Sorry for I didn't noticed that.
All my misunderstanding is base on the old check "--implicit-check-not=foo" :

Command Output (stderr):
--
command line:1:22: error: MAIN-NOT: excluded string found in input
-implicit-check-not='foo'
                     ^
<stdin>:17:32: note: found here
 34: e8 c7 ff ff ff callq -57 <foo>

Thank you very much!

jhenderson added a comment.Apr 14 2020, 1:21 AM
In D77780#1973437, @xiangzhangllvm wrote:

All things clear:
The test have been changed from --implicit-check-not=foo to --implicit-check-not=<foo>: last month.
Sorry for I didn't noticed that.
All my misunderstanding is base on the old check "--implicit-check-not=foo" :

Command Output (stderr):
--
command line:1:22: error: MAIN-NOT: excluded string found in input
-implicit-check-not='foo'
                     ^
<stdin>:17:32: note: found here
 34: e8 c7 ff ff ff callq -57 <foo>

Thank you very much!

No problem. I don't think there's anything that needs doing here, so could you mark this review as "Abandoned" please?

xiangzhangllvm abandoned this revision.Apr 14 2020, 1:36 AM

Done

Revision Contents

PathSize
llvm/
test/
tools/
llvm-objdump/
X86/
2 lines

Diff 256197

llvm/test/tools/llvm-objdump/X86/disassemble-functions.test

## Show that the --diassemble-functions switch disassembles only the specified ## Show that the --diassemble-functions switch disassembles only the specified
## functions. ## functions.
# RUN: yaml2obj -o %t.out %p/Inputs/simple-executable-x86_64.yaml # RUN: yaml2obj -o %t.out %p/Inputs/simple-executable-x86_64.yaml
# RUN: llvm-objdump -d %t.out --disassemble-symbols=main \ # RUN: llvm-objdump -d %t.out --disassemble-symbols=main \
# RUN: | FileCheck %s --check-prefix=MAIN --implicit-check-not="<foo>:" --implicit-check-not=somedata # RUN: | FileCheck %s --check-prefix=MAIN --implicit-check-not=somedata
# RUN: llvm-objdump -d %t.out --disassemble-symbols=main,foo \ # RUN: llvm-objdump -d %t.out --disassemble-symbols=main,foo \
# RUN: | FileCheck %s --check-prefixes=MAIN,FOO --implicit-check-not=somedata # RUN: | FileCheck %s --check-prefixes=MAIN,FOO --implicit-check-not=somedata
# FOO: <foo>: # FOO: <foo>:
# MAIN: <main>: # MAIN: <main>:
## Unknown symbol name. ## Unknown symbol name.
# RUN: llvm-objdump -d %t.out --disassemble-symbols=baz \ # RUN: llvm-objdump -d %t.out --disassemble-symbols=baz \
Show All 11 Lines