Skip to content

Enhance port_tools to be able to do more with each benchmark.#472

Merged
mattmccutchen-cci merged 5 commits into
mainfrom
workflow-alltypes-and-compile
Mar 15, 2021
Merged

Enhance port_tools to be able to do more with each benchmark.#472
mattmccutchen-cci merged 5 commits into
mainfrom
workflow-alltypes-and-compile

Conversation

@mattmccutchen-cci

@mattmccutchen-cci mattmccutchen-cci commented Mar 5, 2021

Copy link
Copy Markdown
Member
  • Add --build_dir option to convert_project to specify a build directory (containing compile_commands.json) different from the source directory (which is scanned for source files to update includes). This is needed by LibArchive and ZLib, which currently set --project_path to the build directory, which breaks include updating.
  • Switch from -output-postfix to -output-dir to reduce the amount of code needed to move converted source files into place for the post-conversion build. Theoretically, this would break any caller that relies on the current location of the output files, but the current benchmark test workflow doesn't use the output files at all.
  • To make the output directory location more predictable, always set the 3C base directory equal to the --project_path specified to convert_project rather than the common ancestor directory of the source files in the compilation database. This will temporarily break LibArchive and ZLib even further until they are migrated to the new usage of the --project_path and --build_dir options.
  • Add --extra-3c-arg to convert_project that can be used to pass the -alltypes flag through to 3C.

mwhicks1/3c-actions#7 will use the new features to improve the benchmark test workflow.

- convert_project: New -extra-3c-arg option (for -alltypes)

- Add compile_converted_project.sh to compile with Checked C clang after
  conversion.

Plus minor cleanups. Notably, when converting all files, have
generate_ccommands use the compilation database rather than taking the
union of the compiler flags.
Comment thread clang/tools/3c/utils/port_tools/compile_converted_project.sh Outdated
Comment thread clang/tools/3c/utils/port_tools/compile_converted_project.sh Outdated
Comment thread clang/tools/3c/utils/port_tools/compile_converted_project.sh Outdated
Comment thread clang/tools/3c/utils/port_tools/generate_ccommands.py Outdated

@mwhicks1 mwhicks1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few comments.

@mattmccutchen-cci mattmccutchen-cci marked this pull request as draft March 11, 2021 00:13
@mattmccutchen-cci mattmccutchen-cci force-pushed the workflow-alltypes-and-compile branch from f2e7ec2 to b2490a8 Compare March 11, 2021 17:14
@mattmccutchen-cci

mattmccutchen-cci commented Mar 11, 2021

Copy link
Copy Markdown
Member Author

Note: I force-pushed the workflow-alltypes-and-compile branch back to where it was before I tried to change it to use -output-dir yesterday so others can continue to use that branch (or temporary merges of it with their own branches) to run the expanded workflow in the interim until I fix the include-updating bug. Of course, this interim version of the expanded workflow has the same include-updating bug as the existing workflow, but the rest of the expansion still provides valuable additional test coverage. I considered making a separate "stable" branch for others to use while I continued to work on this PR, but for the moment, I thought it was simpler to just force-push the PR branch back.

Comment thread clang/tools/3c/utils/port_tools/generate_ccommands.py
Comment thread clang/tools/3c/utils/port_tools/generate_ccommands.py
@john-h-kastner

john-h-kastner commented Mar 11, 2021

Copy link
Copy Markdown
Collaborator

Note: I force-pushed the workflow-alltypes-and-compile branch back to where it was before I tried to change it to use -output-dir yesterday so others can continue to use that branch (or temporary merges of it with their own branches) to run the expanded workflow in the interim until I fix the include-updating bug. Of course, this interim version of the expanded workflow has the same include-updating bug as the existing workflow, but the rest of the expansion still provides valuable additional test coverage. I considered making a separate "stable" branch for others to use while I continued to work on this PR, but for the moment, I thought it was simpler to just force-push the PR branch back.

Does it make sense to merge the PR as is? If the fix doesn't take long it'll be good to have it fixed first, but, as you've said, we're already testing against this PR, so it should be merged.

@mattmccutchen-cci

Copy link
Copy Markdown
Member Author

Does it make sense to merge the PR as is? If the fix doesn't take long it'll be good to have it fixed first, but, as you've said, we're already testing against this PR, so it should be merged.

It's up to you. I think I'll probably have the fix by tomorrow, and I was hoping to avoid the overhead of another two PRs. Another aspect I didn't mention is that in the current PRs, the step that moves the converted files over the originals for the post-conversion build is broken for libarchive and zlib in the same way as the include updating, making the post-conversion build for those two benchmarks meaningless (with or without -alltypes).

@mattmccutchen-cci mattmccutchen-cci marked this pull request as ready for review March 13, 2021 01:56
@mattmccutchen-cci

mattmccutchen-cci commented Mar 13, 2021

Copy link
Copy Markdown
Member Author

I think I have the benchmarks with separate build directories working as intended now: example run. The annoying thing is that now that the workflow is actually doing everything it is supposed to do (including using the debug build of clang from the 3C repository for the initial build of each benchmark), it takes over an hour. I'll be looking for ways to speed it up next (mwhicks1/3c-actions#8), but I don't think we should wait for the speedup before merging the current PRs.

@john-h-kastner john-h-kastner left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@mattmccutchen-cci mattmccutchen-cci merged commit 023d3a4 into main Mar 15, 2021
@mattmccutchen-cci mattmccutchen-cci deleted the workflow-alltypes-and-compile branch March 15, 2021 17:35
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.

3 participants