Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libafl_cc fixes for windows #710

Merged
merged 7 commits into from
Aug 12, 2022
Merged

libafl_cc fixes for windows #710

merged 7 commits into from
Aug 12, 2022

Conversation

abgeana
Copy link
Contributor

@abgeana abgeana commented Jul 25, 2022

This PR includes changes that enable LibAFL's optimization passes on Windows. I think this is not perfect, but could be used as a start.

In order to get these running, one has to:

  • Compile LLVM (tested with version 14.0.6) in order to get llvm-config.exe which is not distributed in the Windows package. I used the following commands
mkdir build; cd build

cmake -G "Visual Studio 17 2022" -A x64             `
    -DLLVM_ENABLE_PROJECTS="clang;compiler-rt;lld"  `
    -DLLVM_EXPORT_SYMBOLS_FOR_PLUGINS=ON            `
    -DLLVM_TARGETS_TO_BUILD=X86 -Thost=x64          `
    ../llvm

cmake --build . --config Release
  • Link the rust code against the static runtime libraries by setting the following environment variable
$env:RUSTFLAGS='-C target-feature=+crt-static' cargo build --release

I made a Dockerfile to build a Windows container here which install the various dependencies required to set things up. I also wrote down some extended notes here.

There is currently a problem that I am still trying to figure out. If I compile stuff with the libafl_cc wrapper and with -fsanitize=address, there is an ACCESS VIOLATION triggered before the target is even started. This gist might be related.

@domenukk
Copy link
Member

Awesome!

@tokatoka
Copy link
Member

Thank you!
#634

@tokatoka
Copy link
Member

I think we want to put your code into a rust's feature.
because we want llvm-config and we need to compile llvm from src, but I guess we don't want to build llvm everytime on ci
so we only enable building llvm & enabling llvm passes on windows when the feature is on
we can turn that feature off for the gh workflow

@domenukk
Copy link
Member

Do we need to compile llvm from source? Can't we take the precompiled binaries and just download the respective headers?
That'd make the process a lot less painful..

@abgeana
Copy link
Contributor Author

abgeana commented Jul 27, 2022

I believe it is not needed, if the information from llvm-config can be supplied differently, for example environment vars. I will change the code to use that and try with the distributed binaries. I will get back to you with the results.

@tokatoka
Copy link
Member

If llvm-config is just for checking the versions of the llvm tools, I think we could just parse the output of clang --version

@domenukk
Copy link
Member

domenukk commented Aug 5, 2022

Hey @abgeana , still working on this or should we eventually take over?
No pressure, just out of curiosity :)

@abgeana
Copy link
Contributor Author

abgeana commented Aug 6, 2022

Hi @domenukk,

I think I reached a conclusion. I changed the code a bit to skip the llvm-config.exe requirement, but there are more things missing that just this binary. For reference, I placed the code in a different branch to not taint this pull request.

Missing stuff includes some headers like llvm\Config\llvm-config.h (and associated llvm-config-x86_64.h) and some libraries. I asked on the LLVM discord about the reasoning and apparently for windows only a toolchain is distributed which is not meant to be linked against.

So unfortunately, it seems that the distributed LLVM package cannot be used for Windows and manually compiling things is needed :/

For CI/CD purposes, an option could also be to create a Docker container which has a compiled version of LLVM readily available, such that building it would not be needed for every CI/CD run. I was able to do everything inside Docker myself, but I'm not sure about licensing stuff since msvc is being used.

@domenukk
Copy link
Member

domenukk commented Aug 6, 2022

Just a stupid idea: is it possible to link against it, anyway, downloading only the headers but using the precompiled binaries?
Else I think any way you chose is fine. :)
Maybe we want to leave the build without full llvm as fallback for people that are in a hurry

@abgeana
Copy link
Contributor Author

abgeana commented Aug 6, 2022

Yeah I thought about a repo+distributed headers combo, but these headers are generated by cmake during the build. They would have been included in the distributed package, but are now not included for windows. So a manually compiled version of llvm would still be needed from somewhere.

I agree that putting this under a feature flag in cargo would be the most straightforward option.

@syheliel
Copy link
Collaborator

syheliel commented Aug 7, 2022

Thanks for your PR! Here are some potential ideas to compile llvm in CI:

  1. use vcpkg install llvm
  2. llvm-tutor has previously written a CI script to compile llvm on windows, it is deleted now but maybe helpful.

@abgeana
Copy link
Contributor Author

abgeana commented Aug 11, 2022

Hi @syheliel,

We decided to change the behavior of build.rs to skip building the passes if no llvm is identified. This is also in line with the previous behavior.

I was not sure why the macos-latest build was failing, but apparently it is also failing in other PRs so it is not related to the contents of my PR in particular.

@domenukk
Copy link
Member

domenukk commented Aug 12, 2022

I can reproduce the CI failure on my Mac, some behavior seems to have changed. It seems to pick up a different clang (XCode instead of brew).
The correct one doesn't error but merely emits a warning (probably also not working as intended @andreafioraldi ?)

clang-14: warning: argument unused during compilation: '-mllvm -granularity=FUNC' [-Wunused-command-line-argument]

/edit: it does seem to pick up the right llvm-config (/opt/homebrew/Cellar/llvm/14.0.6_1/bin/llvm-config)

@syheliel
Copy link
Collaborator

Although not related, maybe we can fix this problem first for macos?

warning: src/cmplog.c:72:15: warning: 'syscall' is deprecated: first deprecated in macOS 10.12 - syscall(2) is unsupported; please switch to a supported interface. For SYS_kdebug_trace use kdebug_signpost(). [-Wdeprecated-declarations]
warning:   valid_len = syscall(SYS_write, dummy_fd[1], ptr, len);
warning:               ^
warning: /Applications/Xcode_13.2.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/unistd.h:746:6: note: 'syscall' has been explicitly marked deprecated here
warning: int      syscall(int, ...);
warning:          ^
warning: 1 warning generated.

@syheliel
Copy link
Collaborator

In https://github.com/AFLplusplus/LibAFL/blob/main/fuzzers/libfuzzer_libpng_accounting/src/bin/libafl_cc.rs:

        if let Some(code) = cc
            .cpp(is_cpp)
            // silence the compiler wrapper output, needed for some configure scripts.
            .silence(true)
            .parse_args(&args)
            .expect("Failed to parse the command line")
            .link_staticlib(&dir, "libfuzzer_libpng")
            .add_arg("-fsanitize-coverage=trace-pc-guard")
            .add_pass(LLVMPasses::CoverageAccounting)
            .add_passes_arg(format!("-granularity={}", GRANULARITY))
            .run()

.add_passes_arg(format!("-granularity={}", GRANULARITY)) causes the problem

@domenukk
Copy link
Member

Although not related, maybe we can fix this problem first for macos?

warning: src/cmplog.c:72:15: warning: 'syscall' is deprecated: first deprecated in macOS 10.12 - syscall(2) is unsupported; please switch to a supported interface. For SYS_kdebug_trace use kdebug_signpost(). [-Wdeprecated-declarations]
warning:   valid_len = syscall(SYS_write, dummy_fd[1], ptr, len);
warning:               ^
warning: /Applications/Xcode_13.2.1.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/unistd.h:746:6: note: 'syscall' has been explicitly marked deprecated here
warning: int      syscall(int, ...);
warning:          ^
warning: 1 warning generated.

We need syscalls, so there's nothing we can do.

@abgeana
Copy link
Contributor Author

abgeana commented Aug 12, 2022

@syheliel and @domenukk thank you for also looking into the CI/CD fails. Seems all checks are working fine now. There is one last bug(?) which popped up, namely the arguments passed to LLVM passes. An example is the aforementioned -granularity=FUNC argument passed to LLVMPasses::CoverageAccounting.

I believe the issue is with how arguments are passed in the old vs new LLVM pass manager. Currently in CI/CD (at least in the MacOS builds), the old pass manager is used regardless of the LLVM version. This is due to the logic behind version identification and the fact that no -14 is appended to llvm-config. This PR includes a function to identify the version based on the output of llvm-config --version or clang --version, but is currently disabled. Some changes are needed to clang.rs to set pass arguments differently.

For now I suggest to leave this PR as is (unless there are some breaking things I'm missing), and have another PR for the aforementioned issue.

@tokatoka
Copy link
Member

tokatoka commented Aug 12, 2022

I can reproduce these errors too with clang-14
If I use
new pass manager -> error
legacy pass manager -> just warnings

@tokatoka
Copy link
Member

tokatoka commented Aug 12, 2022

llvm/llvm-project#56137
seems related oof
I can make a patch to fix it

@tokatoka
Copy link
Member

I can reproduce the CI failure on my Mac, some behavior seems to have changed. It seems to pick up a different clang (XCode instead of brew).
The correct one doesn't error but merely emits a warning (probably also not working as intended @andreafioraldi ?)

I guess this is fine. All these warnings appear on linking not on compiling. when clang is called with -c then no warnings appear.

@domenukk
Copy link
Member

Nice, thx!

@domenukk domenukk merged commit c1aafe3 into AFLplusplus:main Aug 12, 2022
@tokatoka
Copy link
Member

oh, wait.
I want to re-enable this change 1a75ba8 before merging...

@tokatoka
Copy link
Member

tokatoka commented Aug 12, 2022

anyways thank you for your pr! @abgeana

khang06 pushed a commit to khang06/LibAFL that referenced this pull request Oct 11, 2022
* libafl_cc fixes for windows

* libafl_cc checks for llvm-config (again)

* libafl_cc clang-format

* libafl_cc fixes for macos

* maintain libafl_cc pass manager selection logic

* libafl_cc rustfmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants