microsoft / llvm-mctoll

llvm-mctoll
Other
816 stars 125 forks source link

Move command line params to TableGen version like in the current version of the llvm-objdump #177

Closed sv99 closed 2 years ago

sv99 commented 2 years ago

Move from llvm::cl to TableGen version command line check.

Add --sysroot params for working with Mac (my working system).

--include-file - add files from include in the depth - now checked anyway.

sv99 commented 2 years ago

Please, view my PR. I have two next big PR with massive internal vars renaming for satisfy global clang-tidy style.

bharadwajy commented 2 years ago

Thanks! Good code enhancement proposal.

Following are the options related to specifying external function prototypes in the current repo.

--include-files=<string>         - List of comma-seperated header files with function prototypes using standard C syntax.
  -I                             - Alias for --include-files=<single-header-file>

It appears that with this PR we've lost support for --include-files. I see --include-file but it appears different from --include-files which expects a list of comma-separated files.

Build target check-mctoll results in 4 test failures due to this.

Can you please address this?

sv99 commented 2 years ago

I think exists some behavior problems in the --include-files flag.

In the test suite majority tests included system files by full path one by one (-I flag).

Utility internaly scan only this file, but system loaded all included files (system header files have some #include <..>).

https://github.com/microsoft/llvm-mctoll/blob/9a289e20fe972219db98a512d40f5b92b48a94ca/Raiser/IncludedFileInfo.cpp#L152-L169

On the linux system SYSTEMROOT by default is "/" and you don't see warnings about error loading this files - on MAC I have this.

I think more convenient using single custom header file like this

// header-inc.h

#include <stdio.h>
#include <strings.h>
...

and include this file.

sv99 commented 2 years ago

For working with other targets I add --sysroot flag for toolchain path (different from default system path - /)

My working system is MAC - for compiling to linux target and arm target I made toolchain https://github.com/sv99/llvm-mctoll-toolchains

sv99 commented 2 years ago

Change 4 tests

bharadwajy commented 2 years ago

For working with other targets I add --sysroot flag for toolchain path (different from default system path - /)

My working system is MAC - for compiling to linux target and arm target I made toolchain https://github.com/sv99/llvm-mctoll-toolchains

Can you add a (markdown) document in doc directory specifying the steps to be followed for cross compilation on MacOS? This will be of help to developers who might be interested in taking that route.

sv99 commented 2 years ago

I have some changes at work to unify the test suite for working on linux and mac smoothly. In my fork I have two massive changes for renaming internal vars for satisfy global clang-tidy and now I am working on the Arm - problems with calling function.

bharadwajy commented 2 years ago

I have some changes at work to unify the test suite for working on linux and mac smoothly. In my fork I have two massive changes for renaming internal vars for satisfy global clang-tidy and now I am working on the Arm - problems with calling function.

That is great to learn. Since this change introduces a new option --sysroot it would be appropriate to add the necessary documentation to help users - as part of this change. I'd suggest that each of the enhancements be self-contained. This will help during any potential retrospective for bug origins at a later time.

This PR looks good to me, but would be great to add the documentation / usage expectation for the new option --sysroot.

sv99 commented 2 years ago

doc file added

sv99 commented 2 years ago

Strange conflict message! Do I need to do something?

bharadwajy commented 2 years ago

Strange conflict message! Do I need to do something?

I pushed a bug fix exposed when I built the repo on Ubuntu 22.04. Please rebase the PR.

sv99 commented 2 years ago

PR rebased

sv99 commented 2 years ago

doc/sysroot_flag.md corrected Sorry for my english.

bharadwajy commented 2 years ago

doc/sysroot_flag.md corrected Sorry for my english.

Thank you very much. Please do not apologize. I believe progress is made by collaborating.

I'll look through the changes one final time later today.

sv99 commented 2 years ago

Patch applied