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

Replace alloc-related offset type to usize instead isize #838

Merged
merged 4 commits into from
May 29, 2023

Conversation

fepicture
Copy link
Contributor

@fepicture fepicture commented May 26, 2023

EDIT1: fix #674
this pr only replaces the isize to usize for offset in allocation, which makes the code more robust.
there is no further test since a negative will be a failure at compile time.

@fepicture
Copy link
Contributor Author

fepicture commented May 26, 2023

well I have run the .github/script/ci-test.sh, and encountered errors in compiling pfm-sys(third-party lib), I do not think this is related to mmtk, say the full script does not run.

error log
++ [[ x86_64 == \x\8\6\_\6\4 ]]
++ [[ linux == \l\i\n\u\x ]]
++ cargo test --features perf_counter
   Compiling pfm-sys v0.0.14
   Compiling mmtk v0.18.0 (/mmtk-core)
error: failed to run custom build command for `pfm-sys v0.0.14`



Caused by:
  process didn't exit successfully: `/mmtk-core/target/debug/build/pfm-sys-1aa429483a40e9a0/build-script-build` (exit status: 101)
  --- stdout
  OUT_DIR: "/mmtk-core/target/debug/build/pfm-sys-4c4afeaa387dfb3f/out"
  CARGO_MANIFEST_DIR: "/.cargo/registry/src/github.com-1ecc6299db9ec823/pfm-sys-0.0.14"
  Compiling for 'x86_64' target
  Compiling for 'Linux' system
  make[1]: Entering directory '/mmtk-core/target/debug/build/pfm-sys-4c4afeaa387dfb3f/out/libpfm4/lib'
  clang-15 -fPIC  -g -Wall -Werror -Wextra -Wno-unused-parameter -I. -I/mmtk-core/target/debug/build/pfm-sys-4c4afeaa387dfb3f/out/libpfm4/include -DCONFIG_PFMLIB_DEBUG -DCONFIG_PFMLIB_OS_LINUX  -g -Wall -Werror -Wextra -Wno-unused-parameter -I. -I/mmtk-core/target/debug/build/pfm-sys-4c4afeaa387dfb3f/out/libpfm4/lib/../include -DCONFIG_PFMLIB_DEBUG -DCONFIG_PFMLIB_OS_LINUX -D_REENTRANT -I. -fvisibility=hidden -DCONFIG_PFMLIB_ARCH_X86 -DCONFIG_PFMLIB_ARCH_X86_64 -I. -c pfmlib_common.c
  make[1]: Leaving directory '/mmtk-core/target/debug/build/pfm-sys-4c4afeaa387dfb3f/out/libpfm4/lib'

  --- stderr
  pfmlib_common.c:1780:12: error: variable 'n' set but not used [-Werror,-Wunused-but-set-variable]
          int i, u, n = 0, um;
                    ^
  1 error generated.
  make[1]: *** [/mmtk-core/target/debug/build/pfm-sys-4c4afeaa387dfb3f/out/libpfm4/lib/../rules.mk:30: pfmlib_common.o] Error 1
  make: *** [Makefile:49: all] Error 2
  thread 'main' panicked at 'make exited with status exit status: 2', /.cargo/registry/src/github.com-1ecc6299db9ec823/pfm-sys-0.0.14/build.rs:47:9
  stack backtrace:
     0: rust_begin_unwind
               at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/std/src/panicking.rs:575:5
     1: core::panicking::panic_fmt
               at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/core/src/panicking.rs:65:14
     2: build_script_build::main
               at ./build.rs:47:9
     3: core::ops::function::FnOnce::call_once
               at /rustc/90743e7298aca107ddaa0c202a4d3604e29bfeb6/library/core/src/ops/function.rs:251:5
  note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
warning: build failed, waiting for other jobs to finish...
$ ./ci-test.sh 

GitHub mirror old code

source forge latest code have solved this one?

EDIT1:

I got it. CC @caizixian
I use clang-15 to run ci-test.sh which need compile pfm package, and encounter error state the unused var.
pfm is a high-level wrapper about libpfm4.
here libpfm4 recently adds a new commit to remove the unused var which can solve this problem. https://sourceforge.net/p/perfmon2/libpfm4/ci/533633adf7d00bbfcb7f2759567869d585bf97e1/

and our cargo used pfm which is from https://github.com/caizixian/pfm, which actually uses a git submodule to control the internal libpfm4 code, simply updating the submodule commit may solve this? may @caizixian help to update? the submodule i cannot access..caizixian/pfm@7223ed8

@caizixian
Copy link
Member

@fepicture fixed. Please refer to the latest commit.

@fepicture
Copy link
Contributor Author

May you have time to take a review? @qinsoon
the failure ci is public API change, this is normal right?

Btw, finally, I used gcc11 to build pfm(clang15 has other pitches, and the upstream has not changed to fix)

@qinsoon
Copy link
Member

qinsoon commented May 28, 2023

May you have time to take a review? @qinsoon the failure ci is public API change, this is normal right?

The PR looks good. The failure in the API check is expected, as the PR changes the API. I will submit a few PRs to update our bindings for this change. Thanks again for the PR.

@k-sareen
Copy link
Collaborator

k-sareen commented May 28, 2023

Could you change this file as well? The offset is a ssize_t here: https://github.com/mmtk/mmtk-core/blob/c5a20c059674c4eee2c1d005180b1f12a99088e1/vmbindings/dummyvm/api/mmtk.h

@k-sareen
Copy link
Collaborator

Apparently there's another header file here: https://github.com/mmtk/mmtk-core/blob/c5a20c059674c4eee2c1d005180b1f12a99088e1/docs/header/mmtk.h

It should be updated as well.

@fepicture
Copy link
Contributor Author

Apparently there's another header file here: https://github.com/mmtk/mmtk-core/blob/c5a20c059674c4eee2c1d005180b1f12a99088e1/docs/header/mmtk.h

It should be updated as well.

Thank you for your review and for pointing out the omission!
I have made the necessary changes to the file as suggested.
Again, Thank you for your patience and guidance.

@qinsoon
Copy link
Member

qinsoon commented May 29, 2023

binding-refs
OPENJDK_BINDING_REPO=qinsoon/mmtk-openjdk
OPENJDK_BINDING_REF=update-mmtk-core-838
JIKESRVM_BINDING_REPO=qinsoon/mmtk-jikesrvm
JIKESRVM_BINDING_REF=update-mmtk-core-838
JULIA_BINDING_REPO=qinsoon/mmtk-julia
JULIA_BINDING_REF=update-mmtk-core-838

fepicture and others added 4 commits May 29, 2023 10:11
@fepicture
Copy link
Contributor Author

binding-refs OPENJDK_BINDING_REPO=qinsoon/mmtk-openjdk OPENJDK_BINDING_REF=update-mmtk-core-838 JIKESRVM_BINDING_REPO=qinsoon/mmtk-jikesrvm JIKESRVM_BINDING_REF=update-mmtk-core-838 JULIA_BINDING_REPO=qinsoon/mmtk-julia JULIA_BINDING_REF=update-mmtk-core-838

I am really sorry to change the commit info by rebase to solve the out of date,
may those dependencies need extra updates? https://github.com/mmtk/mmtk-openjdk/pull/221/files#diff-99ac2fb415f1a54ae4d65e0487c56226749e91ad44f9fffcddccefa38de8e1a0R33
so If there out-of-date master branch, can I solve this by myself(outer-contributer)?
say we only use merge and do not use rebase to avoid the original commit change?

@qinsoon
Copy link
Member

qinsoon commented May 29, 2023

binding-refs OPENJDK_BINDING_REPO=qinsoon/mmtk-openjdk OPENJDK_BINDING_REF=update-mmtk-core-838 JIKESRVM_BINDING_REPO=qinsoon/mmtk-jikesrvm JIKESRVM_BINDING_REF=update-mmtk-core-838 JULIA_BINDING_REPO=qinsoon/mmtk-julia JULIA_BINDING_REF=update-mmtk-core-838

I am really sorry to change the commit info by rebase to solve the out of date, may those dependencies need extra updates? https://github.com/mmtk/mmtk-openjdk/pull/221/files#diff-99ac2fb415f1a54ae4d65e0487c56226749e91ad44f9fffcddccefa38de8e1a0R33 so If there out-of-date master branch, can I solve this by myself(outer-contributer)? say we only use merge and do not use rebase to avoid the original commit change?

I will update the binding PRs once this mmtk-core PR is merged. Don't worry about it.

Copy link
Member

@qinsoon qinsoon left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the PR.

@qinsoon qinsoon merged commit 2ec37bd into mmtk:master May 29, 2023
13 of 14 checks passed
qinsoon added a commit to mmtk/mmtk-openjdk that referenced this pull request May 29, 2023
qinsoon added a commit to mmtk/mmtk-jikesrvm that referenced this pull request May 29, 2023
qinsoon added a commit to mmtk/mmtk-julia that referenced this pull request May 29, 2023
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.

Allocation offset should be usize
4 participants