Resolver for GSYM format. - #22
Conversation
|
Many thanks for splitting the pull request into smaller bits! Do you intend to address the comments that I raised over in #17? |
c34d6a9 to
4b5b27b
Compare
|
Fixed feedbacks in #17. |
41447ee to
741ed43
Compare
danielocfb
left a comment
danielocfb
left a comment
There was a problem hiding this comment.
Gave it a first pass. Overall it looks okay, but I left a few comments that I think we should address.
| gsym_fo.read_to_end(&mut data).unwrap(); | ||
| let ctx = GsymContext::parse_header(&data).unwrap(); | ||
|
|
||
| assert_eq!(ctx.addr_at(1), 0x00000000004004b0); |
There was a problem hiding this comment.
Similar comment about usage of a linker script as I have below.
There was a problem hiding this comment.
I don't see it. What comment are you saying?
There was a problem hiding this comment.
I don't see it. What comment are you saying?
There was a problem hiding this comment.
Looks like GitHub doesn't work properly.
b1b2301 to
78c54a3
Compare
|
It seems as if CI is red for the change in question. Can you please check @ThinkerYzu1 ? |
5c76553 to
dba1869
Compare
|
Fixed some issues caused by rebase. |
| ctx.header.addr_off_size = 4; | ||
| ctx.header.base_address = 0; | ||
| // Share the same buffer. | ||
| ctx.addr_tab = unsafe { mem::transmute(addr_tab.as_slice()) }; |
There was a problem hiding this comment.
Safety comment is missing and this operation is not safe at all. Rust does not allow the presence of shared reference while a mutable one is outstanding and you create a mutable one below.
There was a problem hiding this comment.
I just wrote a safety comment in the test case. Please check it.
There was a problem hiding this comment.
Unfortunately not. It does not matter if both execute at the same time or not. It's not about data races, it's about aliasing. Can you instantiate a new test case fixture for each iteration, perhaps?
There was a problem hiding this comment.
I am not sure what you expect in safety comments. Rust doesn't like to have mutable and immutable references to the same object at the same time. The problem is not aliasing. The problem is how we use references. Here, the comment is to address the potential problem to explain why it is safe.
By the way, I will create a new instance of ctx every time to avoid this.
| let copy_to_addr_tab = |values: &[u32], addr_tab: &mut [u8]| { | ||
| let mut off = 0; | ||
| for v in values { | ||
| let dst = (&mut addr_tab[off]) as *mut u8; | ||
| let src = v as *const u32 as *const u8; | ||
| unsafe { copy(src, dst, 4) }; | ||
| off += 4; | ||
| } | ||
| }; |
There was a problem hiding this comment.
So you want to write a couple of u32 into a [u8]? You should be able to use Write in conjunction with u32::to_le_bytes. Please avoid unsafe code and pointer casts and arithmetic, if possible.
There was a problem hiding this comment.
Although, we support only LE now. Using u32::to_le_bytes() means this test case will only run with LE platform. So, I will change it to to_ne_bytes().
|
Still seeing outstanding open issues. Will wait until those are addressed before giving it a hopefully final pass. |
df4a2a9 to
0b7c4c4
Compare
|
I have changed the code in the way I mentioned earlier. |
|
a533840 to
3e9d46e
Compare
| * a custom toolchain. However, it is overkilled. | ||
| * | ||
| * The recipe is fragile due to the differences between toolchains and | ||
| * versions. |
There was a problem hiding this comment.
I try to make it happen. However, it is fragile and complicated (a lot of details).
Someone may be able to make it stable and portable.
3e9d46e to
b50e108
Compare
Parse GSYM format to symbolize addresses. Signed-off-by: Kui-Feng Lee <kuifeng@fb.com>
b50e108 to
55c27fe
Compare
4 participants
Parse GSYM format to symbolize addresses.