Page MenuHomePhabricator

[clang][AVR] Reject non assembly source files for the avr1 family
AbandonedPublic

Authored by benshi001 on Jan 24 2022, 7:28 PM.

Details

Diff Detail

Event Timeline

benshi001 created this revision.Jan 24 2022, 7:28 PM
benshi001 requested review of this revision.Jan 24 2022, 7:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 24 2022, 7:28 PM

There is an old solution in https://reviews.llvm.org/D117423. The concern over there is that AVR specific checks should be implemented in AVR toolchain specific files. And this patch obeys that.

I don't know the AVR specific thing but having this toolchain-specific diagnostic in the toolchain-specific file seems the right direction.

clang/lib/Driver/ToolChains/AVR.cpp
418
benshi001 updated this revision to Diff 403116.Jan 25 2022, 7:43 PM
benshi001 marked an inline comment as done.
benshi001 updated this revision to Diff 403867.Jan 27 2022, 8:15 PM
benshi001 added inline comments.
clang/lib/Driver/ToolChains/AVR.cpp
393

Currently command clang xx.c --target=avr -mmcu=attiny40 leads to wrong assembly generated, so we temperarily disable the avrtiny family. Until https://github.com/llvm/llvm-project/issues/53459 is fixed.

MaskRay added a comment.EditedJan 29 2022, 12:51 PM

I do not know whether the complexity is justified. Rejecting some -mmcu= for C source files looks quite dubious. Does it really help users?

Bear in mind that all these complexity translates to clang executable size increase for other users (most people don't use AVR).

I do not know whether the complexity is justified. Rejecting some -mmcu= for C source files looks quite dubious. Does it really help users?

Bear in mind that all these complexity translates to clang executable size increase for other users (most people don't use AVR).

Actually I would like to make clang+llvm+lld fully compatible with gcc+binutils (functionality, ABI, quality, ...), and finally replace avr-gcc as Arduino's default toolchain. (https://www.arduino.cc/en/software, Arduino is a popular MCU platform for hardware prototype development & verification)

Even many avr-libc (https://www.nongnu.org/avr-libc/) developers are trying build it with clang.

If you think it is complex, we can remove the extreme check of -x assembler-with-cpp, which is rarely in a real world.

benshi001 retitled this revision from [AVR][clang] Reject non assembly source files for the avr1 family to [clang][AVR] Reject non assembly source files for the avr1 family.

The other implementation (https://reviews.llvm.org/D117423) is abundoned, since we should not put AVR specified checks in the common code in Clang.cpp.

benshi001 updated this revision to Diff 412300.Mar 1 2022, 6:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2022, 6:30 PM
aykevl added a comment.EditedMar 25 2022, 7:51 AM

@MaskRay it was my suggestion to move this from the toolchain specific file to the generic file, because it makes the implementation much simpler. See my comment D117423#3251110 for details.

Rejecting some -mmcu= for C source files looks quite dubious. Does it really help users?

For context: the avr1 family isn't supported by avr-gcc either. It's a old and rather limited subset of the AVR instruction set. I assume it's going to be rather difficult (and not worth the trouble) to write a C compiler for it, as it doesn't even have a fully functional stack. From Wikipedia:

The AVR1 subset was not popular and no new models have been introduced since 2000. It omits all RAM except for the 32 registers mapped at address 0–31 and the I/O ports at addresses 32–95. The stack is replaced by a 3-level hardware stack, and the PUSH and POP instructions are deleted. All 16-bit operations are deleted, as are IJMP, ICALL, and all load and store addressing modes except indirect via Z.

So I think the idea is to disallow this family so that users won't accidentally try to use a C compiler for these chips. However, writing assembly might still make sense.

@MaskRay it was my suggestion to move this from the toolchain specific file to the generic file, because it makes the implementation much simpler. See my comment D117423#3251110 for details.

Rejecting some -mmcu= for C source files looks quite dubious. Does it really help users?

For context: the avr1 family isn't supported by avr-gcc either. It's a old and rather limited subset of the AVR instruction set. I assume it's going to be rather difficult (and not worth the trouble) to write a C compiler for it, as it doesn't even have a fully functional stack. From Wikipedia:

The AVR1 subset was not popular and no new models have been introduced since 2000. It omits all RAM except for the 32 registers mapped at address 0–31 and the I/O ports at addresses 32–95. The stack is replaced by a 3-level hardware stack, and the PUSH and POP instructions are deleted. All 16-bit operations are deleted, as are IJMP, ICALL, and all load and store addressing modes except indirect via Z.

So I think the idea is to disallow this family so that users won't accidentally try to use a C compiler for these chips. However, writing assembly might still make sense.

My opinion is: we indeed need to be compatible with avr-gcc (accept assembly but reject c progarms), but we do it in AVR specific files and let common code clean.

benshi001 abandoned this revision.May 24 2022, 11:08 PM