Skip to content

Don't substract 1 from end_addr in line program writing#2174

Merged
yurydelendik merged 3 commits intobytecodealliance:mainfrom
bjorn3:patch-2
Sep 7, 2020
Merged

Don't substract 1 from end_addr in line program writing#2174
yurydelendik merged 3 commits intobytecodealliance:mainfrom
bjorn3:patch-2

Conversation

@bjorn3
Copy link
Contributor

@bjorn3 bjorn3 commented Aug 29, 2020

Fixes #2173

@ggreif
Copy link
Contributor

ggreif commented Aug 29, 2020

Thanks for the quick fix! Let me come up with a regression test for it! (Will take a few hours, but there is no haste...) DONE!

@ggreif
Copy link
Contributor

ggreif commented Aug 29, 2020

PR: bjorn3#1

Note: the test added is ignored, due to the usual lack of tools on CI for running it. Try

cargo test translate -- --ignored

After reverting 053757f, the above fails.

Copy link
Contributor

@yurydelendik yurydelendik left a comment

Choose a reason for hiding this comment

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

Let's try that. In the past it was always an issue, e.g. I had the following in the dwarf-to-json library:

https://github.com/yurydelendik/dwarf-to-json/blob/d9c6f665f67da3f862a36d88b86eb488ca4bbc4d/src/dwarf.rs#L488-L500

Can you check if we have similar case here?

I'm skeptical about test having hardcoded address. We can keep it though for now with note that it relies on cranelift generator, so the address will be changed when generation changes.

}
}
let end_addr = (map.offset + map.len - 1) as u64;
let end_addr = (map.offset + map.len) as u64;
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe add a comment similar to what we have in the test?

@ggreif
Copy link
Contributor

ggreif commented Aug 31, 2020

I'm skeptical about test having hardcoded address. We can keep it though for now with note just it relies on cranelift generator, so the address will be justed when generation changes.

Yeah, right, I was thinking of this too. But it shouldn't be very bad, since

  • short code is more resistant of CG changes
  • this part of CG should be mature and rather slowly-moving by now (fingers crossed)
  • the test is ignored, so it won't run in CI
  • the intent is clear, so easy to fix if a CG change would touch this.

Also the FileCheck Rust library seems to be missing advanced features like captures and evaluation. I don't even know whether the LLVM tool proper provides those.

So I suggest to leave it like this for now, and if it causes headaches, just drop it. Better ideas?

Ok(())
}

#[allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a copy&pasted function which could be refactored. I didn't do it, because this is test-only code. If somebody objects, I'm happy to fix.

@bjorn3
Copy link
Contributor Author

bjorn3 commented Aug 31, 2020

Filecheck does have captures:

; check: brnz v0, $(splitEdge=$BB)

@ggreif
Copy link
Contributor

ggreif commented Aug 31, 2020

Filecheck does have captures

That's cool, but I cannot see the ability to do $(expr <var> + 1) or such.

Co-authored-by: Gabor Greif <ggreif@gmail.com>
@ggreif
Copy link
Contributor

ggreif commented Sep 5, 2020

Let's try that. In the past it was always an issue, e.g. I had the following in the dwarf-to-json library:

https://github.com/yurydelendik/dwarf-to-json/blob/d9c6f665f67da3f862a36d88b86eb488ca4bbc4d/src/dwarf.rs#L488-L500

Can you check if we have similar case here?

I can't see how that could be. The DWARF5 standard is pretty much unambiguous in its wording (p151):

end_sequence:
A boolean indicating that the current address is that of the first byte after the end of a sequence of target machine instructions. end_sequence terminates a sequence of lines; therefore other information in the same row is not meaningful.

@yurydelendik yurydelendik merged commit ba9908d into bytecodealliance:main Sep 7, 2020
@yurydelendik
Copy link
Contributor

Thank you for the patch

@bjorn3 bjorn3 deleted the patch-2 branch September 7, 2020 13:44
cfallin pushed a commit that referenced this pull request Nov 30, 2020
* Don't substract 1 from end_addr in line program writing

Fixes #2173

* add testcase for end_sequence having offset past retq (#1)

* Update tests/all/debug/translate.rs

Co-authored-by: Gabor Greif <ggreif@gmail.com>

Co-authored-by: Gabor Greif <ggreif@gmail.com>
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.

lldb cannot symbolise the last retq of functions

3 participants