microsoft / llvm-mctoll

llvm-mctoll
Other
816 stars 125 forks source link

Move to OpaquePointer #183

Closed sv99 closed 2 years ago

sv99 commented 2 years ago

I think the problems in the code base remain exists, but current test suite works completely. Changes in the code marked as "OpaquePointer hack".

bharadwajy commented 2 years ago

Thanks for this PR.

I see the following build failure with the PR changes on my Ubuntu 22.04 dev box:

/github/repo/llvm-project/llvm/tools/llvm-mctoll/MachODump.cpp:3614:21: error: cannot initialize a parameter of type 'LLVMOpInfoCallback' (aka 'int (*)(void *, unsigned long, unsigned long, unsigned long, int, void *)') with an lvalue of type 'int (void *, uint64_t, uint64_t, uint64_t, uint64_t, int, void *)' (aka 'int (void *, unsigned long, unsigned long, unsigned long, unsigned long, int, void *)'): different number of parameters (6 vs 7) [clang-diagnostic-error]
        TripleName, SymbolizerGetOpInfo, SymbolizerSymbolLookUp,
                    ^~~~~~~~~~~~~~~~~~~
/github/repo/llvm-project/llvm/include/llvm/MC/TargetRegistry.h:641:55: note: passing argument to parameter 'GetOpInfo' here
  createMCSymbolizer(StringRef TT, LLVMOpInfoCallback GetOpInfo,
                                                      ^
/github/repo//llvm-project/llvm/tools/llvm-mctoll/MachODump.cpp:3663:28: error: cannot initialize a parameter of type 'LLVMOpInfoCallback' (aka 'int (*)(void *, unsigned long, unsigned long, unsigned long, int, vocmake -S llvm -B build/Debug -G "Ninja" -DLLVM_TARGETS_TO_BUILcmake --build build/Debug/ -- check-mctollD="X86;ARM" -DLLVM_ENABLE_PROJECTS="clang;lld" -DLLVM_ENABLE_ASSERTIONS=true -DCMAKE_BUILD_TYPE=Debug -DMCTOLL_CLANG_TIDY=1id *)') with an lvalue of type 'int (void *, uint64_t, uint64_t, uint64_t, uint64_t, int, void *)' (aka 'int (void *, unsigned long, unsigned long, unsigned long, unsigned long, int, void *)'): different number of parameters (6 vs 7) [clang-diagnostic-error] code on the PR branch
          ThumbTripleNa code on the PR branchme, SymbolizerGetOpInfo, SymbolizerSymbolLookUp,
                           ^~~~~~~~~~~~~~~~~~~
/github/repo/llvm-project/llvm/include/llvm/MC/TargetRegistry.h:641:55: note: passing argument to parameter 'GetOpInfo' here
  createMCSymbolizer(StringRef TT, LLVMOpInfoCallback GetOpInfo,
                                                      ^
5058 warnings and 2 errors generated.
Error while processing /github/repo/llvm-project/llvm/tools/llv code on the PR branchm-mctoll/MachODump.cpp.

Config and build commands used:

cmake -S llvm -B build/Debug -G "Ninja" -DLLVM_TARGETS_TO_BUILD="X86;ARM" -DLLVM_ENABLE_PROJECTS="clang;lld" -DLLVM_ENABLE_ASSERTIONS=true -DCMAKE_BUILD_TYPE=Debug -DMCTOLL_CLANG_TIDY=1

cmake --build build/Debug/ -- check-mctoll

Can you please take a look?

sv99 commented 2 years ago

Last commit 69844fe - need switch llvm-project to tag llvmorg-15.0.1 commit dd472ff - worked without switch llvm-project

bharadwajy commented 2 years ago

Last commit 69844fe - need switch llvm-project to tag llvmorg-15.0.1 commit dd472ff - worked without switch llvm-project

Sorry! My folly. Thanks!

bharadwajy commented 2 years ago

I see 78 failures with this PR.

I had been working on changes on a local branch of mine several weeks ago to use Opaque pointers. As a result, at first glance, I recognize at least some of these failures for which I had fixes on that local branch. I'll check if I can cherrypick those changes and verify that they still work.

For the record following are the failures I see on my Ubuntu 22.04

Failed Tests (78):
  mctoll :: asm_test/X86/discover-ret-type.s
  mctoll :: asm_test/X86/func-xor-three-arg.s
  mctoll :: asm_test/X86/func-xor-two-arg.s
  mctoll :: asm_test/X86/raise-add16rr.s
  mctoll :: asm_test/X86/raise-and.s
  mctoll :: asm_test/X86/raise-basic-globals.s
  mctoll :: asm_test/X86/raise-binary-op-imm-to-reg.s
  mctoll :: asm_test/X86/raise-binary-op-mem-to-reg.s
  mctoll :: asm_test/X86/raise-binary-op-reg-to-reg.s
  mctoll :: asm_test/X86/raise-binary-op-result-to-mem.s
  mctoll :: asm_test/X86/raise-branch-empty-mbb.s
  mctoll :: asm_test/X86/raise-bt.s
  mctoll :: asm_test/X86/raise-call64m.s
  mctoll :: asm_test/X86/raise-call64r-float.s
  mctoll :: asm_test/X86/raise-cmovle.s
  mctoll :: asm_test/X86/raise-cmovns.s
  mctoll :: asm_test/X86/raise-cmovs.s
  mctoll :: asm_test/X86/raise-cmp.s
  mctoll :: asm_test/X86/raise-cmpss-cmpsd.s
  mctoll :: asm_test/X86/raise-cvt.s
  mctoll :: asm_test/X86/raise-divr.s
  mctoll :: asm_test/X86/raise-exit-call.s
  mctoll :: asm_test/X86/raise-imul64r.s
  mctoll :: asm_test/X86/raise-imul64rm.s
  mctoll :: asm_test/X86/raise-jnp.s
  mctoll :: asm_test/X86/raise-lea-global.s
  mctoll :: asm_test/X86/raise-load-from-select-value.s
  mctoll :: asm_test/X86/raise-minmax-float.s
  mctoll :: asm_test/X86/raise-movaps.s
  mctoll :: asm_test/X86/raise-movdq.s
  mctoll :: asm_test/X86/raise-movups.s
  mctoll :: asm_test/X86/raise-or-test.s
  mctoll :: asm_test/X86/raise-or.s
  mctoll :: asm_test/X86/raise-packed-convert.s
  mctoll :: asm_test/X86/raise-packed-fp-ops.s
  mctoll :: asm_test/X86/raise-padd-psub.s
  mctoll :: asm_test/X86/raise-pcmp.s
  mctoll :: asm_test/X86/raise-pmin-pmax.s
  mctoll :: asm_test/X86/raise-popcnt.s
  mctoll :: asm_test/X86/raise-pshufd.s
  mctoll :: asm_test/X86/raise-sal1.s
  mctoll :: asm_test/X86/raise-sar1.s
  mctoll :: asm_test/X86/raise-sarmi.s
  mctoll :: asm_test/X86/raise-sbb.s
  mctoll :: asm_test/X86/raise-shlmi.s
  mctoll :: asm_test/X86/raise-shlrr.c
  mctoll :: asm_test/X86/raise-shr.s
  mctoll :: asm_test/X86/raise-sqrtsd-sqrtss.s
  mctoll :: asm_test/X86/raise-sse-binary-inst.s
  mctoll :: asm_test/X86/raise-sse-ret-types.s
  mctoll :: asm_test/X86/raise-ucomisd-ucomiss.s
  mctoll :: asm_test/X86/raise-unpcklpd.s
  mctoll :: asm_test/X86/raise-vararg-args.s
  mctoll :: asm_test/X86/raise-xor.s
  mctoll :: asm_test/X86/raise-xorps-xorpd.s
  mctoll :: asm_test/X86/test-cwde-test.s
  mctoll :: asm_test/X86/test-reachingdefs-incoming.s
  mctoll :: bug_fixes/issue-70.c
  mctoll :: dhrystone/dhry.test
  mctoll :: smoke_test/eflags-test.c
  mctoll :: smoke_test/externvar-test.c
  mctoll :: smoke_test/fprintf.c
  mctoll :: smoke_test/global-initialized-array-access.c
  mctoll :: smoke_test/global-static-pointer.c
  mctoll :: smoke_test/local-symbol.c
  mctoll :: smoke_test/matrix-back-edge.c
  mctoll :: smoke_test/raise-arg-reaching-def.c
  mctoll :: smoke_test/raise-mov32ri-global-data.c
  mctoll :: smoke_test/raise-non-stack-access-using-bp.c
  mctoll :: smoke_test/raise-ret-reg-reaching-def.c
  mctoll :: smoke_test/raise-switch-init.c
  mctoll :: smoke_test/raise-switch-table.c
  mctoll :: smoke_test/stack-increment.c
  mctoll :: smoke_test/string-split.c
  mctoll :: smoke_test/switch-with-indirect-branch.c
  mctoll :: smoke_test/switch_test-2.c
  mctoll :: smoke_test/test-dynrel-copy-type.c
  mctoll :: smoke_test/test-jmptbl.c

Testing Time: 262.20s
  Unsupported      :  17
  Passed           : 143
  Expectedly Failed:   2
  Failed           :  78
sv99 commented 2 years ago

I think exists some incompatibility in you version and in mine. Clean branch opaque-pointer-step1_rev5 on the ubuntu 20.04 result:

# llvm-mctoll status
# user@vm-ubuntu-204:~/work/llvm-project/llvm/tools/llvm-mctoll [opaque-pointer-step1_rev5]$
user@vm-ubuntu-204:~/work/llvm-project/build-debug [(HEAD detached at llvmorg-15.0.1)]$ ninja check-mctoll
...

--

********************
********************
Unsupported Tests (4):
  mctoll :: smoke_test/ARM/switch_test.c
  mctoll :: unittests/ARM/FuncArgsAndReturn/seven_args.c
  mctoll :: unittests/ARM/FunctionPrototype/no_ret_and_args.c
  mctoll :: unittests/ARM/FunctionPrototype/six_args.c

********************
Failed Tests (6):
  mctoll :: smoke_test/ARM/func-arg-liveness.c
  mctoll :: smoke_test/ARM/global-array-access.c
  mctoll :: smoke_test/ARM/hello.c
  mctoll :: smoke_test/ARM/short.c
  mctoll :: smoke_test/ARM/strcmp-test.c
  mctoll :: smoke_test/ARM/variadic-call-test.c

Testing Time: 109.35s
  Unsupported      :   4
  Passed           : 227
  Expectedly Failed:   3
  Failed           :   6
FAILED: tools/llvm-mctoll/test/CMakeFiles/check-mctoll /home/user/work/llvm-project/build-debug/tools/llvm-mctoll/test/CMakeFiles/check-mctoll
cd /home/user/work/llvm-project/build-debug/tools/llvm-mctoll/test && /usr/bin/python3.8 /home/user/work/llvm-project/build-debug/./bin/llvm-lit --show-unsupported -sv --param llvm_site_config=/home/user/work/llvm-project/build-debug/tools/llvm-mctoll/test/lit.site.cfg --param llvm_unit_site_config=/home/user/work/llvm-project/build-debug/tools/llvm-mctoll/test/Unit/lit.site.cfg /home/user/work/llvm-project/build-debug/tools/llvm-mctoll/test
ninja: build stopped: subcommand failed.

X86 subsystem fully worked.

bharadwajy commented 2 years ago

OK. Before I dig into look at the differences in generated binaries on 22.04 and 20.04 I'd like to make sure we are using (effectively) the same build/test commands. Here are mine:

~/github/oss/llvm-project ((llvmorg-15.0.1))$ pushd llvm/tools/llvm-mctoll
~/github/oss/llvm-project/llvm/tools/llvm-mctoll ~/github/oss/llvm-project
~/github/oss/llvm-project/llvm/tools/llvm-mctoll ((8b32fff...))$ git st
HEAD detached at sv99/opaque-pointer-step1_rev5
nothing to commit, working tree clean
~/github/oss/llvm-project/llvm/tools/llvm-mctoll ((8b32fff...))$ popd
~/github/oss/llvm-project
~/github/oss/llvm-project ((llvmorg-15.0.1))$ cmake -S llvm -B build/Debug -G "Ninja" -DLLVM_TARGETS_TO_BUILD="X86;ARM" -DLLVM_ENABLE_PROJECTS="clang;lld" -DLLVM_ENABLE_ASSERTIONS=true -DCMAKE_BUILD_TYPE=Debug -DMCTOLL_CLANG_TIDY=1
<output snipped>
~/github/oss/llvm-project ((llvmorg-15.0.1))$ cmake --build build/Debug/ -- check-mctoll
<output snipped>
Testing Time: 204.98s
  Unsupported      :  17
  Passed           : 143
  Expectedly Failed:   2
  Failed           :  78

We'll need to fix these 78 failures on Ubuntu 22.04 as well as the 6 on Ubuntu 20.04 before merging the PR changes.

sv99 commented 2 years ago

Problem in the Clang 15.0.0 Build System Changes CMake -DCLANG_DEFAULT_PIE_ON_LINUX=ON is now the default.

Worked workaround for this problem configure CMAKE with -DCLANG_DEFAULT_PIE_ON_LINUX=OFF

My worked CMAKE config

CC=clang-13 CXX=clang++-13 cmake -S llvm -B build-debug -G "Ninja" \
  -DLLVM_TARGETS_TO_BUILD="X86;ARM"  \
  -DLLVM_ENABLE_PROJECTS="clang;lld" \
  -DCMAKE_BUILD_TYPE=Debug           \
  -DLLVM_USE_LINKER=lld \
  -DBUILD_SHARED_LIBS=ON \
  -DLLVM_OPTIMIZED_TABLEGEN=ON \
  -DCMAKE_CXX_COMPILER_LAUNCHER=ccache \
  -DCLANG_DEFAULT_PIE_ON_LINUX=OFF

After this changes all X86 test worked.

Exists problems with ARM subsystem, but this problems not with opaque pointers. I very interested in the ARM subsystem. I have some additional commits in my fork for correcting problems with ARM subsystem.

bharadwajy commented 2 years ago

Thanks for identifying the cause. I have verified that all x64 tests do pass on Ubuntu 22.04 upon configuring with -DCLANG_DEFAULT_PIE_ON_LINUX=OFF. Can you please add this information to README.md?

After this PR is merged, I'll file an issue and continue making the necessary changes to address the multiple x64 mctoll test failures seen when built using clang/llvm 15 tree - in which -DCLANG_DEFAULT_PIE_ON_LINUX=ON is the default.

I am reviewing the changes.

sv99 commented 2 years ago

I have PR which unified test suite for MAC - in this PR added -no-pie flag in the test suite.

sv99 commented 2 years ago

README changed

sv99 commented 2 years ago

Merging??

bharadwajy commented 2 years ago

Merging??

Give me a couple of days to complete code review. Had been extra busy with my day job :-)

sv99 commented 2 years ago

I think you may merge PR as is and change X86 subsystem how do you see it. Now I made very much changes in the ARM sybsystem in my fork.

sv99 commented 2 years ago

My PR is the initial step in the moving to the opaque pointer.

sv99 commented 2 years ago

What do you think about merging this big PR "as is" and change some think in the next PR.

bharadwajy commented 2 years ago

What do you think about merging this big PR "as is" and change some think in the next PR.

I would like to address issues that are identified (whether cosmetic or stylistic or those related to cleaner, less hack-y implementation) in a PR it is merged. That way we will not lose the context to properly handle them as we move on to add new features / address other issues. As a result, I have been working on additions to this PR that implement the suggestions I made along with additional related ones.

Unfortunately, I fell ill over the past couple of weeks and have still not fully recovered. I hope to be able to wrap up this PR by the upcoming weekend - health permitting.

bharadwajy commented 2 years ago

Pushed. Thanks for these changes and your patience. Much appreciated.