This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by benshi001 on Jan 16 2022, 2:25 AM.

Details

Summary

Devices belongs to the avr1 family do not has SRAM, so
only assembly source files should be accepted. This behaviour
conforms to GCC.

Diff Detail

Event Timeline

benshi001 created this revision.Jan 16 2022, 2:25 AM
benshi001 requested review of this revision.Jan 16 2022, 2:25 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 16 2022, 2:25 AM

This is GCC's handling on AVR

https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/config/avr/avr.c;h=62973927fdc30d502e5d225f83cdde958bf2dad0;hb=refs/heads/master#l10424

around line 10433 / function static void avr_file_start (void),

gcc rises an error architecture %qs supported for assembler only

benshi001 updated this revision to Diff 400375.Jan 16 2022, 7:10 AM
benshi001 added inline comments.Jan 16 2022, 7:12 AM
clang/lib/Driver/ToolChains/AVR.cpp
419

We need not care about '-x c++' or '-x c', which are excluded by above checks.

benshi001 added inline comments.Jan 16 2022, 7:14 AM
clang/lib/Driver/ToolChains/AVR.cpp
417

In this for loop, the key point is which one is found first.

It is OK that -x assembler-with-cpp comes first, otherwise an error should be reported.

I'm not sure this is the correct location for these checks. You're essentially checking whether the compilation looks like a C/C++ compilation or an assembly compilation based on the flags and the file name. However, the Clang driver already does something like this: it converts the command line arguments and files into a list of jobs to perform. This is done in Driver::BuildActions, Driver::BuildJobs, Clang::ConstructJob, and other places.
I think a better place to do this check is in Clang::ConstructJob. There is already something similar here:

https://github.com/llvm/llvm-project/blob/10ed1eca241f893085b8db40138e588e72aaee3a/clang/lib/Driver/ToolChains/Clang.cpp#L4396-L4398

// C++ is not supported for IAMCU.
if (IsIAMCU && types::isCXX(Input.getType()))
  D.Diag(diag::err_drv_clang_unsupported) << "C++ for IAMCU";

I think something like this will work, in Clang::ConstructJob:

bool IsAVR = ...
...

if (IsAVR) {
    D.Diag(diag::err_drv_clang_unsupported) << "C/C++ for AVR";

There is a different ConstructJob for assembly, so this works.

clang/lib/Driver/ToolChains/AVR.h
22

I think I would have called this CPU not AVRMcu, but AVRMcu is fine.

clang/test/Driver/avr-mmcu.S
4 ↗(On Diff #400375)

Is there a reason why you used {{.*}} instead of the actual value?

benshi001 updated this revision to Diff 401982.Jan 21 2022, 7:35 AM
benshi001 added a comment.EditedJan 21 2022, 7:42 AM

Thanks for your suggestion ! I have changed to check avr-1 in Clang::ConstructJob, which is much more clear than my previous solution !

Really appreicate. A bit pity is that I have to make big changes to the constructor AVRToolChain::AVRToolChain, otherwise the expected error *** only supports assembly programming can not rise if -c is specified in clang options. Because in previous code, the reading of FamilyName is embraced by !Args.hasArg(options::OPT_c), something like

if (!Args.hasArg(options::OPT_c)) {
    Optional<StringRef> FamilyName = GetMCUFamilyName(CPU);
}

so I have to re-arrange the code logic.

! In D117423#3251110, @aykevl wrote:

I'm not sure this is the correct location for these checks. You're essentially checking whether the compilation looks like a C/C++ compilation or an assembly compilation based on the flags and the file name. However, the Clang driver already does something like this: it converts the command line arguments and files into a list of jobs to perform. This is done in Driver::BuildActions, Driver::BuildJobs, Clang::ConstructJob, and other places.
I think a better place to do this check is in Clang::ConstructJob. There is already something similar here:

https://github.com/llvm/llvm-project/blob/10ed1eca241f893085b8db40138e588e72aaee3a/clang/lib/Driver/ToolChains/Clang.cpp#L4396-L4398

// C++ is not supported for IAMCU.
if (IsIAMCU && types::isCXX(Input.getType()))
  D.Diag(diag::err_drv_clang_unsupported) << "C++ for IAMCU";

I think something like this will work, in Clang::ConstructJob:

bool IsAVR = ...
...

if (IsAVR) {
    D.Diag(diag::err_drv_clang_unsupported) << "C/C++ for AVR";

There is a different ConstructJob for assembly, so this works.

benshi001 added a comment.EditedJan 21 2022, 7:58 AM

The previous logic of the constructor AVRToolChain::AVRToolChain

  1. if -c or -nostdlib is specified, no error will rise (even if no MCU is specified).
  1. if no MCU is specified, two warnings will rise
warning: no target microcontroller specified on command line
warning: standard library not linked and so no interrupt vector table
  1. if the specified avr device has no proper avr family name, two warnings will rise
warning: support for linking stdlibs for microcontroller '%0' is not implemented
warning: standard library not linked and so no interrupt vector table
  1. if no available avr-gcc is found, two warnings will rise
warning: no avr-gcc installation can be found on the system,
warning: standard library not linked and so no interrupt vector table
  1. if no available avr-libc is found, two warnings will rise
warning: no avr-libc installation can be found on the system
warning: standard library not linked and so no interrupt vector table

And after my re-arrangement of the constructor, the above points 2/3/4/5 do not change. But only point 1 changes to
if -c or -nostdlib is specified and a valid avr mcu is specified, no error will rise.
if -c or -nostdlib is specified but no valid avr mcu is specified, a warning will rise (no target microcontroller specified on command line)

benshi001 updated this revision to Diff 402338.Jan 23 2022, 8:22 AM
benshi001 marked an inline comment as done.
benshi001 added inline comments.
clang/lib/Driver/ToolChains/AVR.h
22

It is changed to FamilyName, since family is checked other than the device name.

benshi001 updated this revision to Diff 402379.Jan 23 2022, 5:35 PM
benshi001 marked an inline comment as done.
MaskRay added inline comments.Jan 24 2022, 3:12 PM
clang/lib/Driver/ToolChains/Clang.cpp
5149 ↗(On Diff #402379)

Try moving the check from the generic file to the toolchain-specific file.

benshi001 marked an inline comment as done.Jan 24 2022, 7:33 PM
benshi001 added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
5149 ↗(On Diff #402379)

I have created another patch to do this check in avr toolchain specific files.

https://reviews.llvm.org/D118095

which is much more complex than current solution.

But I do see there are other target specific checks in the common Clang::ConstructJob, such as

https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L5153

https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L5019

https://github.com/llvm/llvm-project/blob/main/clang/lib/Driver/ToolChains/Clang.cpp#L5257

benshi001 marked an inline comment as done.Jan 24 2022, 7:35 PM