Page MenuHomePhabricator

[AVR] Use the correct address space for non-prototyped function calls
ClosedPublic

Authored by aykevl on Apr 14 2020, 9:24 AM.

Details

Summary

Some function declarations like this:

void foo();

do not have a type declaration, for that you'd use:

void foo(void);

Clang internally bitcasts the variadic function declaration to a function pointer, but doesn't use the correct address space on AVR. This commit fixes that.

This fix is necessary to let Clang compile compiler-rt for AVR.


Two remarks on this diff:

  1. I wasn't sure whether there is a way to set the address space on a function. I at least couldn't seem to get it to work. If it is possible to set the address space, I think the fix is slightly wrong and should somehow get the address space from the function declaration instead of using getProgramAddressSpace (please let me know how or where to find this information).
  2. I hoped there was a test file already where I could include this test, but at least CodeGen/address-space.c seemed a bit too complex to also test with AVR (for example, int is 16-bit on AVR).

Diff Detail

Event Timeline

aykevl created this revision.Apr 14 2020, 9:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2020, 9:24 AM
aykevl edited the summary of this revision. (Show Details)Apr 14 2020, 9:31 AM
rjmccall added inline comments.Apr 14 2020, 12:18 PM
clang/lib/CodeGen/CGExpr.cpp
5051

CalleePtr->getType()->getPointerAddressSpace() should be fine.

aykevl updated this revision to Diff 257548.Apr 14 2020, 4:04 PM

Update to use the function pointer address space from the function declaration.

(This is slightly different from what @rjmccall suggested but I think this is more readable).

This revision is now accepted and ready to land.Apr 14 2020, 4:50 PM

This fix is necessary to let Clang compile compiler-rt for AVR.

This suggests that some compiler-rt declarations do not follow the best practice... What are they?

clang/test/CodeGen/address-space-avr.c
4

Add a comment what happens here?

This fix is necessary to let Clang compile compiler-rt for AVR.

This suggests that some compiler-rt declarations do not follow the best practice... What are they?

The calls come from __addXf3__ (in fp_add_impl.inc). This is what I get when I dump the bitcast:

i16 () addrspace(1)* bitcast (i16 (...) addrspace(1)* @__fe_getround to i16 () addrspace(1)*)
i16 () addrspace(1)* bitcast (i16 (...) addrspace(1)* @__fe_raise_inexact to i16 () addrspace(1)*)
i16 () addrspace(1)* bitcast (i16 (...) addrspace(1)* @__fe_getround to i16 () addrspace(1)*)
i16 () addrspace(1)* bitcast (i16 (...) addrspace(1)* @__fe_raise_inexact to i16 () addrspace(1)*)

So they must have been introduced with D57143.

rjmccall added inline comments.Apr 14 2020, 8:16 PM
clang/test/CodeGen/address-space-avr.c
4

Yeah, it'd be nice to add a comment like "Test that we call unprototyped functions from nonzero address spaces correctly." When we drop element types from pointer types, this code will need to change: FunctionPointer will start carrying an llvm::FunctionType* directly, and this code will just change that type instead of creating a bitcast. But it will still be important to test that we handle function pointers in non-zero address spaces correctly.

aykevl updated this revision to Diff 257687.Apr 15 2020, 6:00 AM
aykevl marked an inline comment as done.
  • added comment explaining the test case
This revision was automatically updated to reflect the committed changes.