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.

Sign up for GitHub

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

Jump to bottom

ci: check the prefixing policy for runtime symbols #9260

Merged
merged 5 commits into from
Jan 29, 2020
Merged

ci: check the prefixing policy for runtime symbols #9260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
895d8a0
ci: check runtime symbol policy
Octachron Jan 24, 2020
2dba8a7
tune symbol test: ignore a and allow Caml_ prefix
Octachron Jan 24, 2020
13dc03e
Add appveyor test
Octachron Jan 27, 2020
21292fa
Add exceptions in check-symbol-names:
Octachron Jan 27, 2020
48661a7
refined exceptions for runtime symbol policy
Octachron Jan 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
14 changes: 12 additions & 2 deletions tools/check-symbol-names
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,23 @@ set -o pipefail

[ -z "$*" ] && { echo "Usage: $0 libfoo.a" 1>&2; exit 2; }

nm -A -P "$@" | awk '
nm -A -P "$@" | LC_ALL=C awk '
# ignore caml_foo, camlFoo_bar, _caml_foo, _camlFoo_bar
$2 ~ /^(_?caml[_A-Z])/ { next }
# ignore local and undefined symbols
$3 ~ /^[rbdtsU]$/ { next }
$3 ~ /^[a-zU]$/ { next }
Copy link
Contributor

Choose a reason for hiding this comment

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

Character ranges and case are weird because of posix and locales. (See gawk docs for a lament). Changing [rbdtsU] to [a-zU] sounds good, but to make it reliable we need to change awk to LC_ALL=C awk.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed - see also #8986!

# ignore "main", which should be externally linked
$2 ~ /^_?main$/ { next }
$2 ~ /^_?wmain$/ { next }
# Caml_state escapes the prefixing rule for now
$2 ~ /^_?Caml_state$/ { next }
# for x86 PIC mode
$2 ~ /^__x86.get_pc_thunk./ { next }
# for mingw32
$2 ~ /^.debug_/ { next }
# windows unicode support
$2 ~ /^_win_multi_byte_to_wide_char$/ { next }
$2 ~ /^_win_wide_char_to_multi_byte$/ { next }
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't find any reason why these two exported functions are not prefixed with caml_. @dra27: any opinion?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's a hangover from the original patch, which used win_ for its C stubs. There's no reason not to change this in another PR, as you say. The two functions are exported inside CAML_INTERNALS and there's no reason to use them directly unless you're writing stuff which is explicitly for Windows (you want caml_copy_string_of_os and caml_stat_strdup_to_os normally, as documented in the manual) - a search on GitHub suggests they're not being used anywhere.

# print the rest
{ found=1; print $1 " " $2 " " $3 }
# fail if there were any results
Expand Down
5 changes: 5 additions & 0 deletions tools/ci/appveyor/appveyor_build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,11 @@ case "$1" in
test)
FULL_BUILD_PREFIX="$APPVEYOR_BUILD_FOLDER/../$BUILD_PREFIX"
run 'ocamlc.opt -version' "$FULL_BUILD_PREFIX-$PORT/ocamlc.opt" -version
if [[ $PORT = 'mingw32' ]] ; then
run "Check runtime symbols" \
"$FULL_BUILD_PREFIX-$PORT/tools/check-symbol-names" \
$FULL_BUILD_PREFIX-$PORT/runtime/*.a
fi
run "test $PORT" make -C "$FULL_BUILD_PREFIX-$PORT" tests
run "install $PORT" make -C "$FULL_BUILD_PREFIX-$PORT" install
if [[ $PORT = 'msvc64' ]] ; then
Expand Down
2 changes: 2 additions & 0 deletions tools/ci/travis/travis-ci.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,8 @@ EOF
$MAKE world.opt
$MAKE ocamlnat
fi
echo Ensuring that all names are prefixed in the runtime
./tools/check-symbol-names runtime/*.a
cd testsuite
echo Running the testsuite with the normal runtime
$MAKE all
Expand Down