This is an archive of the discontinued LLVM Phabricator instance.

[AVR] Fix global references to function symbols
ClosedPublic

Authored by amikhalev on Sep 14 2020, 1:34 PM.

Details

Summary

References to functions are in program memory and need a pm() fixup. This should fix trait objects for Rust on AVR.

Diff Detail

Event Timeline

amikhalev created this revision.Sep 14 2020, 1:34 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 14 2020, 1:34 PM
amikhalev requested review of this revision.Sep 14 2020, 1:34 PM
amikhalev updated this revision to Diff 291670.Sep 14 2020, 1:37 PM

Add more context to diff

amikhalev set the repository for this revision to rG LLVM Github Monorepo.
aykevl added a subscriber: aykevl.Sep 14 2020, 5:48 PM

As a quick review (without looking into this carefully): you need to add tests. Without tests this will not be accepted. It also gives a better idea of what it does exactly and thus makes reviewing easier.

amikhalev updated this revision to Diff 292034.Sep 15 2020, 3:10 PM

I added tests. One makes sure that a pm() asm expr generates a R_AVR_16_PM relocation. The other is pruned LLVM IR from a simple Rust program that uses trait objects.

dylanmckay requested changes to this revision.Sep 30 2020, 7:22 AM

Good bugfix - thanks for the second patch!

llvm/lib/Target/AVR/AVRAsmPrinter.cpp
187

This check is not fully sufficient for covering all cases - namely, the constant may not be a function but still might be constrained to program memory and require the new pm fixup. Motivating example: a global variable that is explicitly located in program memory via an addrspace(1) qualifier.

Suggest replacing the "is a function" check with GV->getAddressSpace() == AVR::ProgramMemory which more directly captures what you're wanting to check, and will also add the new pm fixup to regular progmem vars

llvm/lib/Target/AVR/MCTargetDesc/AVRELFObjectWriter.cpp
11

Order the #include alphabetically

llvm/test/CodeGen/AVR/rust-trait-object.ll
10

Rename Rust identifier names in this file to something human readable. For this particular line, a more appropriate name might be EmptyType or simple TypeFoo. In general, LLVM tests are committed without the machine-readable naming conventions of the frontend used to generate them. It will also make some of the CHECK lines shorter and more obvious.

65

Remove attribute specifiers (#n) after functions - most if not all of them should be unnecessary

72

Drop the linking options and calling convention fastcc - I suspect these are not needed to reproduce

141

Delete the attributes which are not required for your test to pass

This revision now requires changes to proceed.Sep 30 2020, 7:22 AM
dylanmckay added inline comments.Sep 30 2020, 7:28 AM
llvm/test/CodeGen/AVR/rust-trait-object.ll
5

Optional: It might be a good idea to post the version of the Rust compiler used if it's not too much hassle to dig that up again.

67

Drop unnecessary function attributes as I suspect they are not required to reproduce the issue

92

Avoid numeric variable names in LLVM tests as they make modfications to the test in the future more difficult as LLVM verifier requires that all numerical vars are defined in order, which makes insertion of new variables mid-function problematic.

There is an existing LLVM pass which can be used to do this automatically, although there are very few numbered variable names in the test already so it might be easier to fixup manually. In the past I've been recommended and used to good success is the instnamer pass, invokable via opt -instnamer ..., although it will strip out your CHECK lines.

Rahix added a subscriber: Rahix.Jan 24 2021, 3:38 AM
This revision was not accepted when it landed; it landed in state Needs Revision.Feb 9 2021, 3:42 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

I have applied the code review suggestions and committed this to master in 2ccb941740e608a9cf70a7c5840497149654b0f6.

This will fix https://github.com/rust-lang/rust/issues/79889

Thanks for the patch @amikhalev!