Clang 16 upgrade #48

ragusaa · 2023-12-08T19:19:44Z

No description provided.

micahsnyder

Initial review just going through cmakelists.txt and clambc-compiler.py. Lotta dead code to clean up, and some stuff that was disabled/deleted for testing that should be restored (namely the 4 cmake files).

I'll look at the optpass code changes after the break, which I suspect is what you actually wanted me to look at (and not this 😅).

CMakeLists.txt

clambcc/clambc-compiler.py

libclambcc/ClamBCAnalyzer/CMakeLists.txt

libclambcc/ClamBCChangeMallocArgSize/ClamBCChangeMallocArgSize.cpp

micahsnyder · 2024-01-04T22:37:35Z

It would be good to update the example pass code to use the new style, as well. In https://github.com/Cisco-Talos/clamav-bytecode-compiler/tree/main/example

We should update the CMakeLists.txt / add the new CMakeLists.txt for the object library and shared library. And change all the #include " to #include <. Etc.

libclambcc/ClamBCAnalyzer/ClamBCAnalyzer.h

libclambcc/Common/CMakeLists.txt

micahsnyder · 2024-01-04T22:53:10Z

There are still a handful of places where I see #include "llvm instead of #include <llvm:

ifacegen.cpp (broken module, due for overhaul and migration to clamav repo. But it couldn't hurt to switch to the other #include style.)
HelloWorld.cpp (dated, need to update example)
ClamBCPreserveABIs.cpp (some commented out, some not. Can we deleted the commented ones?)
ClamBCRegAlloc.cpp (commented out headers. Perhaps delete)
libclambcc/Common/ClamBCModule.h (delete file?)
libclambcc/Common/ClamBCRegAlloc.cpp (delete file?)

micahsnyder · 2024-01-04T23:01:53Z

There are a bunch of #if 0-blocks and it may be time to remove them.

micahsnyder · 2024-01-12T21:31:00Z

Looks like the github workflow also needs to be updated to use clang-16 from clang-8:
https://github.com/Cisco-Talos/clamav-bytecode-compiler/actions/runs/7480532064/job/20360149066
https://github.com/Cisco-Talos/clamav-bytecode-compiler/blob/Clang-16-Upgrade/.github/workflows/cmake.yml#L143

micahsnyder · 2024-01-12T21:33:29Z

To get the clang-format test to pass, we should also upgrade this to use clang-format-16 with the latest version of the github actions plugin, which will be v4.11.0:
https://github.com/Cisco-Talos/clamav-bytecode-compiler/blob/Clang-16-Upgrade/.github/workflows/clang-format.yml#L29-L31

libclambcc/ClamBCAnalyzer/ClamBCAnalyzer.cpp

libclambcc/ClamBCAnalyzer/ClamBCAnalyzer.h

clambcc/clambc-compiler.py

examples/PassManager/AnalysisPlugin/CMakeLists.txt

examples/LegacyPassManager/CMakeLists.txt

examples/LegacyPassManager/HelloWorld/HelloWorld.cpp

examples/PassManager/AnalysisPlugin/AnalysisPlugin.cpp

libclambcc/ClamBCRemoveFreezeInsts/ClamBCRemoveFreezeInsts.cpp

libclambcc/ClamBCRemoveUSUB/ClamBCRemoveUSUB.cpp

libclambcc/ClamBCRemoveUnsupportedICMPIntrinsics/ClamBCRemoveUnsupportedICMPIntrinsics.cpp

libclambcc/ClamBCWriter/ClamBCWriter.cpp

libclambcc/Common/ClamBCUtilities.h

Co-authored-by: Micah Snyder <[email protected]>

…nsupportedICMPIntrinsics.cpp Co-authored-by: Micah Snyder <[email protected]>

micahsnyder · 2024-02-09T17:25:29Z

All of the CI tests are failing:

cmake build is failing because the selected OS doesn't have clang-16. We could install from https://apt.llvm.org/ to ensure it is available.
clang-format-16 is failing, because it expects a space between // or /* and comment bodies.
jenkins is failing with some network error in the freebsd 12.1 jail. Possibly because jails/iocage is janky. If Judge is indeed moving us to use Docker for sigmanager, perhaps we can abandon freebsd in the test-bcc pipeline and test our Docker image instead.

examples/PassManager/CMakeLists.txt

ragusaa · 2024-02-09T17:32:08Z

All of the CI tests are failing:

cmake build is failing because the selected OS doesn't have clang-16. We could install from https://apt.llvm.org/ to ensure it is available.

clang-format-16 is failing, because it expects a space between // or /* and comment bodies.

jenkins is failing with some network error in the freebsd 12.1 jail. Possibly because jails/iocage is janky. If Judge is indeed moving us to use Docker for sigmanager, perhaps we can abandon freebsd in the test-bcc pipeline and test our Docker image instead.

clam-format was using clang-format-12. I fixed it and re-formatted, so the formatting errors should go away.

.github/workflows/cmake.yml

…de-compiler into Clang-16-Upgrade_2

micahsnyder

I think you should also delete the Dockerfile. We now maintain that in the clamav-docker repo, and having one here confuses things.

micahsnyder · 2024-02-13T18:14:30Z

I just added a commit that restructures the libclambcc directory and CMakeLists.txt to have all opt-passes' source code (and thus build objects) located in the same directory instead of subdirectories.

This fixes the unit tests, because they rely on LD_LIBRARY_PATH to specify a directory where the opt-pass libs are located.

I think you should also delete the Dockerfile. We now maintain that in the clamav-docker repo, and having one here confuses things.

Since I made a commit to fix the unit tests, I snuck in this change as well, deleting the Dockerfile.

To test the docker build, will need to grab the Dockerfile from the clamav-docker repo. I'll put in a PR for that in a moment. I have that working as well.

micahsnyder · 2024-02-13T18:16:18Z

Here's the clamav-docker PR: Cisco-Talos/clamav-docker#42

The unit tests rely on all of the opt pass libraries being located in the same directory as libclambccommon.so, as specified using LD_LIBRARY_PATH. One solution would have been to specify a path for each opt pass library in LD_LIBRARY_PATH and then search for each library in each of those LD_LIBRARY_PATH paths. However this felt very clunky. Instead, this commit solves the problem by placing the source for all opt passes in the same directory, which will mean that the .so's created in the build directory will also be co-located and thus using a single path for LD_LIBRARY_PATH will suffice. To make present and future copy-pasting easy, I also renamed the libraries to be camel-case.

NEWS.md

micahsnyder · 2024-04-22T21:14:21Z

The clang-format-16 check failed

micahsnyder

I'm good with this. BCC test pipeline results seemed good, though we did find some improvements needed to the test pipeline itself.

ragusaa force-pushed the Clang-16-Upgrade_2 branch 2 times, most recently from afac373 to 18efdb4 Compare December 19, 2023 18:09

micahsnyder requested changes Dec 22, 2023

View reviewed changes

micahsnyder requested changes Jan 4, 2024

View reviewed changes

libclambcc/ClamBCAnalyzer/CMakeLists.txt Outdated Show resolved Hide resolved

micahsnyder reviewed Jan 4, 2024

View reviewed changes

libclambcc/ClamBCChangeMallocArgSize/ClamBCChangeMallocArgSize.cpp Outdated Show resolved Hide resolved

micahsnyder reviewed Jan 4, 2024

View reviewed changes

libclambcc/ClamBCAnalyzer/ClamBCAnalyzer.h Outdated Show resolved Hide resolved

micahsnyder reviewed Jan 4, 2024

View reviewed changes

libclambcc/Common/CMakeLists.txt Outdated Show resolved Hide resolved

ragusaa force-pushed the Clang-16-Upgrade_2 branch 2 times, most recently from dd0a8e3 to c682c3c Compare January 19, 2024 19:29

micahsnyder requested changes Jan 23, 2024

View reviewed changes

micahsnyder requested changes Feb 2, 2024

View reviewed changes

ragusaa and others added 10 commits February 2, 2024 13:18

Upgraded bytecode compiler to work with Clang/LLVM 16

2620713

Update examples/PassManager/AnalysisPlugin/CMakeLists.txt

7eb71f3

Co-authored-by: Micah Snyder <[email protected]>

Update examples/LegacyPassManager/CMakeLists.txt

54145ca

Co-authored-by: Micah Snyder <[email protected]>

Fixed formatting for examples

0504713

Update examples/LegacyPassManager/HelloWorld/HelloWorld.cpp

de14fbf

Co-authored-by: Micah Snyder <[email protected]>

Update examples/PassManager/AnalysisPlugin/AnalysisPlugin.cpp

b6fe70a

Co-authored-by: Micah Snyder <[email protected]>

Update libclambcc/ClamBCRemoveFreezeInsts/ClamBCRemoveFreezeInsts.cpp

3b7e5c6

Co-authored-by: Micah Snyder <[email protected]>

Update examples/PassManager/CMakeLists.txt

84937a9

Co-authored-by: Micah Snyder <[email protected]>

Update libclambcc/ClamBCRemoveUSUB/ClamBCRemoveUSUB.cpp

7516556

Co-authored-by: Micah Snyder <[email protected]>

Update libclambcc/ClamBCRemoveUnsupportedICMPIntrinsics/ClamBCRemoveU…

3c5556a

…nsupportedICMPIntrinsics.cpp Co-authored-by: Micah Snyder <[email protected]>

ragusaa force-pushed the Clang-16-Upgrade_2 branch from 30353a8 to 3c5556a Compare February 2, 2024 21:44

micahsnyder requested changes Feb 9, 2024

View reviewed changes

examples/PassManager/CMakeLists.txt Outdated Show resolved Hide resolved

Formatted with clang-format-16

15b1b64

Applied patch from Micah Snyder to fix github workflows

3066a9e

micahsnyder reviewed Feb 9, 2024

View reviewed changes

.github/workflows/cmake.yml Outdated Show resolved Hide resolved

micahsnyder and others added 3 commits February 9, 2024 12:53

Update .github/workflows/cmake.yml

d5c20c2

Hopefully fixes tests on github

947fe30

Merge branch 'Clang-16-Upgrade_2' of github.com:ragusaa/clamav-byteco…

e629523

…de-compiler into Clang-16-Upgrade_2

micahsnyder requested changes Feb 13, 2024

View reviewed changes

micahsnyder mentioned this pull request Feb 13, 2024

Clambc compiler 1.4.0 Cisco-Talos/clamav-docker#42

Merged

micahsnyder force-pushed the Clang-16-Upgrade_2 branch from 0ffd5b6 to d4e870a Compare February 13, 2024 18:57

micahsnyder force-pushed the Clang-16-Upgrade_2 branch from d4e870a to c51e641 Compare February 15, 2024 15:25

micahsnyder changed the title ~~Clang 16 upgrade 2 (not ready to merge, only for testing)~~ Clang 16 upgrade Feb 15, 2024

ragusaa force-pushed the Clang-16-Upgrade_2 branch from cacc014 to 714acca Compare March 20, 2024 17:09

Updated NEWS.md

0c2153f

ragusaa force-pushed the Clang-16-Upgrade_2 branch from 191ae99 to 0c2153f Compare March 20, 2024 21:03

micahsnyder reviewed Mar 21, 2024

View reviewed changes

NEWS.md Outdated Show resolved Hide resolved

ragusaa force-pushed the Clang-16-Upgrade_2 branch from 5224d2f to b46684c Compare May 3, 2024 16:05

micahsnyder approved these changes May 8, 2024

View reviewed changes

Fixed error message, as well as bug fixes found in regression

c376689

ragusaa force-pushed the Clang-16-Upgrade_2 branch from 67929e4 to c376689 Compare May 8, 2024 17:43

micahsnyder merged commit 2beadb7 into Cisco-Talos:main May 9, 2024
2 of 3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clang 16 upgrade #48

Clang 16 upgrade #48

ragusaa commented Dec 8, 2023

micahsnyder left a comment •

edited

Loading

micahsnyder commented Jan 4, 2024

micahsnyder commented Jan 4, 2024

micahsnyder commented Jan 4, 2024

micahsnyder commented Jan 12, 2024 •

edited

Loading

micahsnyder commented Jan 12, 2024

micahsnyder commented Feb 9, 2024

ragusaa commented Feb 9, 2024

micahsnyder left a comment

micahsnyder commented Feb 13, 2024

micahsnyder commented Feb 13, 2024

micahsnyder commented Apr 22, 2024

micahsnyder left a comment

Clang 16 upgrade #48

Clang 16 upgrade #48

Conversation

ragusaa commented Dec 8, 2023

micahsnyder left a comment • edited Loading

Choose a reason for hiding this comment

micahsnyder commented Jan 4, 2024

micahsnyder commented Jan 4, 2024

micahsnyder commented Jan 4, 2024

micahsnyder commented Jan 12, 2024 • edited Loading

micahsnyder commented Jan 12, 2024

micahsnyder commented Feb 9, 2024

ragusaa commented Feb 9, 2024

micahsnyder left a comment

Choose a reason for hiding this comment

micahsnyder commented Feb 13, 2024

micahsnyder commented Feb 13, 2024

micahsnyder commented Apr 22, 2024

micahsnyder left a comment

Choose a reason for hiding this comment

micahsnyder left a comment •

edited

Loading

micahsnyder commented Jan 12, 2024 •

edited

Loading