sunfishcode / llvm2cranelift

LLVM IR to Cranelift IR translator
Apache License 2.0
36 stars 4 forks source link

Update for Cretonne reloc changes. #4

Closed djg closed 6 years ago

djg commented 6 years ago

As a precursor to starting on clang2cretonne, I've been trying to get llvm2cretonne to compile. These are the last errors I ran into from changes to Cretonne reloc handling.

@sunfishcode, you might already have local changes that fix this, but I've chosen to pass the addend along with the offset in the relocs tuple. Should addend be added to offset instead?

sunfishcode commented 6 years ago

@djg Keeping the addend separate is the right way to go here. The offset refers to where in the code the relocation applies to, while the addend is added to the symbol value that is written there.

djg commented 6 years ago

Thanks for review @sunfishcode. I'll update the patches soon.

On 23 Dec. 2017 09:53, "Dan Gohman" notifications@github.com wrote:

@sunfishcode commented on this pull request.

This looks good. I indeed don't have local mods for this yet, so thanks for updating this!

In lib/llvm/src/module.rs https://github.com/sunfishcode/llvm2cretonne/pull/4#discussion_r158569854 :

@@ -69,8 +69,11 @@ impl<'a> fmt::Display for DisplayCompiledFunction<'a> { write!(f, "0x{:02x}, ", byte)?; } write!(f, "\n")?;

  • for &(ref reloc, ref name, ref offset) in &compilation.relocs.relocs {
  • write!(f, "reloc: {}:{}@{}\n", reloc, name, offset)?;
  • for &(ref reloc, ref name, ref offset, addend) in &compilation.relocs.relocs {
  • match addend {
  • 0 => write!(f, "reloc: {}:{}@{}\n", reloc, name, offset)?,
  • _ => write!(f, "reloc: {}:{}@{}{}\n", reloc, name, offset, addend)?,

Because the addend is added to the symbol value, it's best to print it after the name, rather than after the offset.

In lib/llvm/src/reloc_sink.rs https://github.com/sunfishcode/llvm2cretonne/pull/4#discussion_r158569897 :

 ) {
  • self.relocs.push((reloc, name.clone(), offset));
  • // TODO: How should addend be handled? Should it be added to
  • // offset and stored in self.relocs or carried through beside
  • // offset?

Carried through. Your code is doing the right thing.

In src/llvm.rs https://github.com/sunfishcode/llvm2cretonne/pull/4#discussion_r158569991 :

             // FIXME: What about other types of relocs?

// TODO: Faerie API: Seems like it might be inefficient to // identify the caller by name each time. // TODO: Faerie API: It's inconvenient to keep track of which // symbols are imports and which aren't.

  • // TODO: How should addend be handled? Should it be added to offset here?

You can pass the added to faerie below, replacing the 0 in the addend: 0 below, which should do everything needed.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/sunfishcode/llvm2cretonne/pull/4#pullrequestreview-85409662, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJhrcn4fEL1fBkHAEfVdbxGbqX3g6J1ks5tDEDtgaJpZM4RKiP1 .

djg commented 6 years ago

@sunfishcode, I updated based on your comments. This should be good to merge now.

sunfishcode commented 6 years ago

Great, thanks!