Details
Diff Detail
Unit Tests
| Time | Test | |
|---|---|---|
| 90 ms | x64 debian > LLVM.Bindings/Go::go.test |
Event Timeline
avr-gcc checks whether the device supports the flash bank used. For example:
$ cat test.c int d = 5; const int ro = 5; __flash const int f = 5; __flash1 const int f1 = 5; __flash2 const int f2 = 5; $ avr-gcc -mmcu=attiny84 -Os -c -o test.o test.c test.c:4:20: error: variable ‘f1’ located in address space ‘__flash1’ beyond flash of 64 KiB __flash1 const int f1 = 5; ^ test.c:5:20: error: variable ‘f2’ located in address space ‘__flash2’ beyond flash of 64 KiB __flash2 const int f2 = 5; ^
It does not appear that this patch has a similar check, while I think that would be useful. Or did you leave it out intentionally?
I have updated my patch, in the newly added test clang/test/Sema/avr-flash.c, all __flash1/__flash2/__flash3/__flash4/__flash5 are denied when running clang against at90s8515 (which belongs to the avr-2 family).
I intentionally using at90s8515 instead of attiny84 in my test case, due to the compiler generates wrong code (use of register r0~r15 can be generated) for the avrtiny family. Actually I plan to
- deny compiling c code for the avr-0 family, and only assembly code is accepted by clang, that behavior matches avr-gcc's.
- Temporarily disable compiling c code for the avr-tiny family, report an error like "c code is not supported on avrtiny currently", until the support of avr-tiny is fully done.
Sorry, it might be that I have misunderstood your concern. Yes, I did add a test for checking the rejection of unsupported flash banks, as shown in the test file clang/test/Sema/avr-flash.c.
The best error message should be something like address space ‘__flash2’ is not supported on current device, but actually the real error in my patch is unknown type name '__flash2', so I write a TOTO line for that
// TODO: It would be better to report "'__flash5' is not supported on at908515".
I will try to fix it in my future patches, and current patch implements
- support __flashN on different devices, and deny __flashN that beyond the real flash size of user specified device (though the error message is inexact)
- generate corresponding address space attributes for __flashN
So I hope current patch can be accepted and I will fix the inaccurate error message in the future.
Looks good to me!
| clang/lib/Basic/Targets/AVR.cpp | ||
|---|---|---|
| 27 | This works and is fine, but I think you could also name it NumFlashBanks so that it is the number of flash banks supported on the device (0 if LPM/ELPM is not supported). With 0 for the attiny11 (which has no accessible flash banks) and 1 for the attiny22 (because it has only one flash bank that can be accessed: flash bank 0). Just a suggestion, the current system looks good to me. | |
| clang/lib/CodeGen/TargetInfo.cpp | ||
| 8290 | These hardcoded numbers are a bit unfortunate, but OK. | |
| clang/test/Sema/avr-flash.c | ||
| 11 | I think this can be implemented by always setting the __flash* defines and checking whether they are valid for the current device in getGlobalVarAddressSpace. | |
| clang/lib/Basic/Targets/AVR.cpp | ||
|---|---|---|
| 27 | I will change it to NumFlashBanks when committing. | |
| clang/test/Sema/avr-flash.c | ||
|---|---|---|
| 11 | Maybe it is better to do in getGlobalVarAddressSpace for reporting the exact source position. a.c:1:20: error: variable ‘arr’ located in address space ‘__flash5’ beyond flash of 64 KiB __flash5 int const arr[8]; I will try it in the next patch. | |
This works and is fine, but I think you could also name it NumFlashBanks so that it is the number of flash banks supported on the device (0 if LPM/ELPM is not supported). With 0 for the attiny11 (which has no accessible flash banks) and 1 for the attiny22 (because it has only one flash bank that can be accessed: flash bank 0).
Just a suggestion, the current system looks good to me.