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

Tools for assessing the risk of changes to the MPS #177

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

rptb1
Copy link
Member

@rptb1 rptb1 commented Feb 24, 2023

This is primarily a tool to help with resolving #110 by assessing the risk to Configura of changes since version 1.115. However, we'd like to develop this into a general-purpose tool. Then we can use it as input to review, for example.

See also the checklist at #127 (comment) which is partially automated by such a tool.

See also #167 (comment) which discusses how developers can know about risks of updating the critical path.

@rptb1
Copy link
Member Author

rptb1 commented Feb 24, 2023

llvm-diff could be used to detect changes to functions, using clang to generate intermediate code. A quick test on my Ubuntu 22 looked promising. Use clang -S -emit-llvm to generate .ll files that can be compared. The output of llvm-diff is quite parseable for function changes. It may be necessary to disable optimization.

@rptb1
Copy link
Member Author

rptb1 commented Feb 25, 2023

llvm-diff could be used to detect changes to functions

Hax.

I put the following script in a file called git-lldiff:

#!/bin/bash
case "$1" in
    *.c)
	llvm-diff-14 \
	    <(clang -o - -I code -S -emit-llvm "$2") \
	    <(clang -o - -I code -S -emit-llvm "$5") 2>&1 |
	    sed -ne 's/^in function \(.*\):$/\1/p'
	;;
esac
exit 0

Then, for example, GIT_EXTERNAL_DIFF=$(pwd)/git-lldiff git show --ext-diff e307cb4f0ac2dc00c6f0b67c44a201e1fcf307b2 produces a list of changed C functions.

It's far from perfect because it uses the current contents of the code directory to compile against, and that may well be inconsistent with the file versions being compared. To work properly the script needs to check out the tree temporarily. Later!

@rptb1
Copy link
Member Author

rptb1 commented Feb 25, 2023

It's far from perfect because it uses the current contents of the code directory

EDIT: Don't use this hack. See https://github.com/Ravenbrook/mps/blob/branch/2023-02-24/risk-tool/tool/git-fundiff

Late night hax. This in git-lldiff:

#!/bin/bash

usage() {
    echo "$0 <git checkout pathspec> <git checkout pathspec> <C source>"
}

left="$1"
shift
right="$1"
shift

tmpdir=$(mktemp --directory)
trap "rm -rf -- '$tmpdir'" EXIT
mkdir -p "$tmpdir/left" "$tmpdir/right"
git --work-tree="$tmpdir/left" checkout "$left" -- code
git --work-tree="$tmpdir/right" checkout "$right" -- code

while test $# -gt 0; do
    file="$1"
    shift
    clang -S -emit-llvm -o "$tmpdir/left.ll" -I "$tmpdir/left/code" "$tmpdir/left/code/$file"
    clang -S -emit-llvm -o "$tmpdir/right.ll" -I "$tmpdir/right/code" "$tmpdir/right/code/$file"
    llvm-diff-14 "$tmpdir/left.ll" "$tmpdir/right.ll" 2>&1 |
	sed -ne 's/^in function \(.*\):$/\1/p'
done

then, for example, ./git-lldiff c23173d78^ c23173d78 $(git show --pretty="format:" --name-only c23173d7 | sed -ne 's!^code/\(.*\.c\)$!\1!p') | sort -u lists all the C functions changed in that commit.

@rptb1
Copy link
Member Author

rptb1 commented Feb 25, 2023

Unfortunately llvm-diff isn't very good. It reports changes to functions even when comparing a file with itself, e.g. llvm-diff x x. Submitted llvm/llvm-project#60993

@rptb1 rptb1 changed the title Tool for assessing the risk of changes to the MPS Tools for assessing the risk of changes to the MPS Feb 25, 2023
@thejayps
Copy link
Contributor

JPH+RB: We need to determine how useful this tool is now or might be in future. There's no need to merge this now, this can live on a branch as something we did. Worthwhile to consider this more: the consideration is "optional". Tool has served its purpose for now

@thejayps thejayps added optional Will cause failures / of benefit. Worth assigning resources. needs analaysis The issue needs analysis before it can be resolved. labels Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs analaysis The issue needs analysis before it can be resolved. optional Will cause failures / of benefit. Worth assigning resources.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants