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

How to count unsafe #419

Closed
Medowhill opened this issue Feb 16, 2021 · 4 comments
Closed

How to count unsafe #419

Medowhill opened this issue Feb 16, 2021 · 4 comments
Assignees

Comments

@Medowhill
Copy link
Collaborator

cargo-count에 버그가 있어 현재는 @efenniht 님이 만든 도구를 사용해 unsafe 줄 수를 세고 있습니다(#314). 해당 도구는 unsafe 함수가 있는 경우 해당 함수의 모든 줄을 unsafe 줄로 간주합니다. 그러나 rv6에서는 unsafe 함수 안에서도 unsafe한 기능을 사용하려면 unsafe 블록을 열어야 하도록 설정(#377)했기 때문에, unsafe 함수 안에 unsafe 블록이 있을 시 그 블록 안에 있는 줄이 두 번 세지는 문제가 있습니다. 이로 인해 일부 파일은 unsafe 줄 비율이 100%가 넘어가는 것으로 측정되기도 합니다(kernel-rs/src/trap.rs: 162/154 = 105%).

최소한 unsafe 함수 안에서는 다시 unsafe 블록을 세지 않도록 함으로써 unsafe 줄을 중복해서 세는 문제가 없도록 고쳐야 합니다.

또한, unsafe 함수 안에서도 unsafe 블록이 필요한 현 상태에서 unsafe 함수의 줄 수를 세는 것이 올바른 측정 방법인지 의문이 듭니다. Unsafe 함수는 호출할 때 지켜야 할 가정이 있다는 것을 의미하는데, 어차피 해당 가정에 대한 reasoning은 함수를 호출하는 곳에서 이루어지며, 함수의 몸통과는 관계가 없습니다. Unsafe 줄 수가 formal 또는 informal reasoning의 비용과 어느 정도 비례한다는 가정하에 unsafe 줄 수를 세는 것일 텐데, unsafe 함수의 줄 수는 빼고 세는 것이 그 가정에 더 잘 부합할 수도 있어 보입니다.

@travis1829
Copy link
Collaborator

travis1829 commented Feb 16, 2021

제 생각에도 unsafe함수라고 해서 그 함수의 모든 줄을 세는 건 맞지 않다고 생각합니다.
또, 말씀하신대로 unsafe 함수의 safety constraints들은 caller가 새로운 unsafe block안에서 체크해야하는 것이라고 생각합니다.

참고로, unsafe block과 unsafe fn의 차이점에 대해서는 unsafe_block_in_unsafe_fn RFC에도 다음과 같이 나와있는 것 같습니다.

unsafe {} blocks are about discharging obligations, but unsafe fn are about defining obligations.

특히, 저희가 unsafe한 부분을 최대한 줄이려는 이유가, 프로그래머가 manual하게 체크해야되는 부분을 줄이기 위해서라고 생각되는데, 이런 관점에서는 unsafe 함수 안에서도 unsafe block 내부 부분만 세는게 맞지 않나 싶습니다.

이와 관련해서 unsafe_block_in_unsafe_fn RFC 문서를 참고해봐도 좋을 것 같습니다.

@Medowhill
Copy link
Collaborator Author

참고로, unsafe 함수의 줄 수를 세지 않으면 다음과 같이 나옵니다.

kernel-rs/src/virtio_disk.rs: 2/287 = 0%
kernel-rs/src/fcntl.rs: 0/10 = 0%
kernel-rs/src/start.rs: 19/59 = 32%
kernel-rs/src/sysproc.rs: 3/50 = 6%
kernel-rs/src/console.rs: 4/165 = 2%
kernel-rs/src/page.rs: 6/65 = 9%
kernel-rs/src/trap.rs: 29/153 = 18%
kernel-rs/src/kernel.rs: 32/168 = 19%
kernel-rs/src/plic.rs: 6/22 = 27%
kernel-rs/src/sysfile.rs: 5/344 = 1%
kernel-rs/src/param.rs: 0/15 = 0%
kernel-rs/src/exec.rs: 3/152 = 1%
kernel-rs/src/pinned_array.rs: 3/26 = 11%
kernel-rs/src/pipe.rs: 24/176 = 13%
kernel-rs/src/bio.rs: 11/120 = 9%
kernel-rs/src/file.rs: 1/192 = 0%
kernel-rs/src/vm.rs: 18/517 = 3%
kernel-rs/src/list.rs: 20/72 = 27%
kernel-rs/src/uart.rs: 11/155 = 7%
kernel-rs/src/syscall.rs: 5/87 = 5%
kernel-rs/src/spinlock.rs: 25/218 = 11%
kernel-rs/src/memlayout.rs: 0/34 = 0%
kernel-rs/src/riscv.rs: 107/342 = 31%
kernel-rs/src/arena.rs: 21/322 = 6%
kernel-rs/src/etrace.rs: 0/117 = 0%
kernel-rs/src/lib.rs: 0/71 = 0%
kernel-rs/src/virtio.rs: 2/87 = 2%
kernel-rs/src/stat.rs: 0/10 = 0%
kernel-rs/src/utils.rs: 0/6 = 0%
kernel-rs/src/poweroff.rs: 3/10 = 30%
kernel-rs/src/sleeplock.rs: 7/100 = 7%
kernel-rs/src/sleepablelock.rs: 7/87 = 8%
kernel-rs/src/kalloc.rs: 5/47 = 10%
kernel-rs/src/proc.rs: 75/591 = 12%
kernel-rs/src/fs/inode.rs: 26/524 = 4%
kernel-rs/src/fs/path.rs: 5/94 = 5%
kernel-rs/src/fs/superblock.rs: 1/37 = 2%
kernel-rs/src/fs/mod.rs: 1/112 = 0%
kernel-rs/src/fs/log.rs: 3/142 = 2%
Total:
490/5786 = 8%

@jeehoonkang 교수님, 이 이슈에 의견 있으신가요?

@Medowhill Medowhill self-assigned this Mar 8, 2021
@Medowhill
Copy link
Collaborator Author

Hafnium도 같은 flag 적용하고 같은 방식으로 unsafe 줄 세어 봅시다

@coolofficials
Copy link
Collaborator

Hafnium 빌드 과정에서 오류가 생겨서 상욱 님께 문의드리는 중입니다.

ghost pushed a commit that referenced this issue Mar 9, 2021
436: Closes #419 r=Medowhill a=Medowhill

이미 count-unsafe를 설치하신 분들은 `cargo uninstall count-unsafe`를 통해 기존의 count-unsafe를 삭제해 주셔야 합니다.

Co-authored-by: Jaemin Hong <[email protected]>
@ghost ghost closed this as completed in 22ef9a9 Mar 9, 2021
Gabriel4256 pushed a commit to Gabriel4256/rv6 that referenced this issue Sep 8, 2021
This issue was closed.
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

No branches or pull requests

3 participants