[clang][AVR] Add avr-libc/include to clang system include paths

[benshi001]

The wrong stdio.h (/usr/include/stdio.h) is included with a simple a.c like

#include <stdio.h>
...

However the expected one is avr-libc's stdio.h (/usr/lib/avr/include), and this patch add avr-libc's include/ to clang system include paths.

[aykevl]

Looks reasonable to me. But again, I would like this to be reviewed also by someone familiar with the internals of Clang (I'm not).

[benshi001]

In D97669#2596073, @aykevl wrote:

Looks reasonable to me. But again, I would like this to be reviewed also by someone familiar with the internals of Clang (I'm not).

Actually this patch is copied from msp430, with just a little change.

[benshi001]

Although the bug can be avoided via "-I /usr/lib/avr/include" in clang's command line option, I expect clang to have the save behaviour as avr-gcc.

And for avr-gcc, avr-gcc a.c -mmcu=xxx will automatically include avr-libc's header files and link avr-libc's libxxxx.a .

[Anastasia]

Is stdio.h used by anything?

clang/lib/Driver/ToolChains/AVR.cpp

356

function name doesn't adhere to the coding style.

[benshi001]

clang/lib/Driver/ToolChains/AVR.cpp

356

Not sure your concern ? Other platforms also use the same function name.

void RISCVToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
                                               ArgStringList &CC1Args) const {
void Linux::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
                                      ArgStringList &CC1Args) const {
void WebAssembly::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
                                            ArgStringList &CC1Args) const {

[benshi001]

In D97669#2661560, @Anastasia wrote:

Is stdio.h used by anything?

No. stdio.h is not used. But if I do #include <avr/interrupt.h>, then -I /usr/lib/avr/include must be specified in the command line option. While avr-gcc does not reqiures that.

I would like to keep clang & avr-gcc in the behaviour.

[Anastasia]

clang/lib/Driver/ToolChains/AVR.cpp

356

https://llvm.org/docs/CodingStandards.html#name-types-functions-variables-and-enumerators-properly

Function names should be verb phrases (as they represent actions), and command-like function should be imperative. The name should be camel case, and start with a lower case letter (e.g. openFile() or isFoo()).

You should adhere to the coding style in the new functionality.

[benshi001]

clang/lib/Driver/ToolChains/AVR.cpp

356

I have created another patch for that, and would like to keep current patch simple without noise.

https://reviews.llvm.org/D100022

[Anastasia]

clang/lib/Driver/ToolChains/AVR.cpp

356

Sorry, my mistake - I haven't realized that you are overriding the method instead of adding a new one. Then I think it is best to keep the old style because we don't normally do purely formatting/renaming changes.

[benshi001]

clang/lib/Driver/ToolChains/AVR.cpp

356

Thanks. You are appreciated to go on with this patch.

[Anastasia]

In D97669#2665865, @benshi001 wrote:

In D97669#2661560, @Anastasia wrote:

Is stdio.h used by anything?

No. stdio.h is not used. But if I do #include <avr/interrupt.h>, then -I /usr/lib/avr/include must be specified in the command line option. While avr-gcc does not reqiures that.

I would like to keep clang & avr-gcc in the behaviour.

Ok, do you plan to use it later? Then perhaps you should be adding it in the subsequent patches?

[benshi001]

In D97669#2676826, @Anastasia wrote:

In D97669#2665865, @benshi001 wrote:

In D97669#2661560, @Anastasia wrote:

Is stdio.h used by anything?

No. stdio.h is not used. But if I do #include <avr/interrupt.h>, then -I /usr/lib/avr/include must be specified in the command line option. While avr-gcc does not reqiures that.

I would like to keep clang & avr-gcc in the behaviour.

Ok, do you plan to use it later? Then perhaps you should be adding it in the subsequent patches?

No. No future pathes are needed. The avr-libc includes standard libc (headers and libs) and avr specific headers and libs. This patch fixes all of these issues. Both standard libc and avr platform specific libs can be used without any explict command line option, just like avr-gcc does.

[Anastasia]

In D97669#2678460, @benshi001 wrote:

In D97669#2676826, @Anastasia wrote:

In D97669#2665865, @benshi001 wrote:

In D97669#2661560, @Anastasia wrote:

Is stdio.h used by anything?

No. stdio.h is not used. But if I do #include <avr/interrupt.h>, then -I /usr/lib/avr/include must be specified in the command line option. While avr-gcc does not reqiures that.

I would like to keep clang & avr-gcc in the behaviour.

Ok, do you plan to use it later? Then perhaps you should be adding it in the subsequent patches?

No. No future pathes are needed. The avr-libc includes standard libc (headers and libs) and avr specific headers and libs. This patch fixes all of these issues. Both standard libc and avr platform specific libs can be used without any explict command line option, just like avr-gcc does.

Ok, so your patch is adding the following file

clang/test/Driver/Inputs/basic_avr_tree/usr/lib/avr/include/stdio.h

But it doesn't seem to be used at present? If you don't need it anywhere it should not be added.

[benshi001]

In D97669#2685698, @Anastasia wrote:

In D97669#2678460, @benshi001 wrote:

In D97669#2676826, @Anastasia wrote:

In D97669#2665865, @benshi001 wrote:

In D97669#2661560, @Anastasia wrote:

Is stdio.h used by anything?

No. stdio.h is not used. But if I do #include <avr/interrupt.h>, then -I /usr/lib/avr/include must be specified in the command line option. While avr-gcc does not reqiures that.

I would like to keep clang & avr-gcc in the behaviour.

Ok, do you plan to use it later? Then perhaps you should be adding it in the subsequent patches?

No. No future pathes are needed. The avr-libc includes standard libc (headers and libs) and avr specific headers and libs. This patch fixes all of these issues. Both standard libc and avr platform specific libs can be used without any explict command line option, just like avr-gcc does.

Ok, so your patch is adding the following file

clang/test/Driver/Inputs/basic_avr_tree/usr/lib/avr/include/stdio.h

But it doesn't seem to be used at present? If you don't need it anywhere it should not be added.

No. The stdio.h is still needed even it is not referred by any code in current patch. It works as a placeholder for the directory clang/test/Driver/Inputs/basic_avr_tree/usr/lib/avr/include/, and this directory is needed in the test case clang/test/Driver/avr-toolchain.c of current patch, and this directory can not exist without stdio.h.

[Anastasia]

In D97669#2687419, @benshi001 wrote:

In D97669#2685698, @Anastasia wrote:

In D97669#2678460, @benshi001 wrote:

In D97669#2676826, @Anastasia wrote:

In D97669#2665865, @benshi001 wrote:

In D97669#2661560, @Anastasia wrote:

Is stdio.h used by anything?

No. stdio.h is not used. But if I do #include <avr/interrupt.h>, then -I /usr/lib/avr/include must be specified in the command line option. While avr-gcc does not reqiures that.

I would like to keep clang & avr-gcc in the behaviour.

Ok, do you plan to use it later? Then perhaps you should be adding it in the subsequent patches?

No. No future pathes are needed. The avr-libc includes standard libc (headers and libs) and avr specific headers and libs. This patch fixes all of these issues. Both standard libc and avr platform specific libs can be used without any explict command line option, just like avr-gcc does.

Ok, so your patch is adding the following file

clang/test/Driver/Inputs/basic_avr_tree/usr/lib/avr/include/stdio.h

But it doesn't seem to be used at present? If you don't need it anywhere it should not be added.

No. The stdio.h is still needed even it is not referred by any code in current patch. It works as a placeholder for the directory clang/test/Driver/Inputs/basic_avr_tree/usr/lib/avr/include/, and this directory is needed in the test case clang/test/Driver/avr-toolchain.c of current patch, and this directory can not exist without stdio.h.

Do you know why an empty directory is not sufficient?

Overall perhaps it makes more sense if someone with more experience on AVR reviews this change.

[benshi001]

In D97669#2688771, @Anastasia wrote:

In D97669#2687419, @benshi001 wrote:

In D97669#2685698, @Anastasia wrote:

In D97669#2678460, @benshi001 wrote:

In D97669#2676826, @Anastasia wrote:

In D97669#2665865, @benshi001 wrote:

In D97669#2661560, @Anastasia wrote:

Is stdio.h used by anything?

No. stdio.h is not used. But if I do #include <avr/interrupt.h>, then -I /usr/lib/avr/include must be specified in the command line option. While avr-gcc does not reqiures that.

I would like to keep clang & avr-gcc in the behaviour.

Ok, do you plan to use it later? Then perhaps you should be adding it in the subsequent patches?

No. No future pathes are needed. The avr-libc includes standard libc (headers and libs) and avr specific headers and libs. This patch fixes all of these issues. Both standard libc and avr platform specific libs can be used without any explict command line option, just like avr-gcc does.

Ok, so your patch is adding the following file

clang/test/Driver/Inputs/basic_avr_tree/usr/lib/avr/include/stdio.h

But it doesn't seem to be used at present? If you don't need it anywhere it should not be added.

No. The stdio.h is still needed even it is not referred by any code in current patch. It works as a placeholder for the directory clang/test/Driver/Inputs/basic_avr_tree/usr/lib/avr/include/, and this directory is needed in the test case clang/test/Driver/avr-toolchain.c of current patch, and this directory can not exist without stdio.h.

Do you know why an empty directory is not sufficient?

Overall perhaps it makes more sense if someone with more experience on AVR reviews this change.

I just learnt that llvm repo uses an empty .keep file to keep an empty directory, so I replaced the stdio.h with a .keep. Thanks for your comments.

[benshi001]

@efriedma

Could you please help me review this patch? It relates to the one you have reviewed for me, and is also about wrong mangled function name in c++ on AVR.

The previous patch has correctly handled __UINT8/16/32/64_TYPE__, but current clang-avr still be wrong at int8/16/32/64_t, the underlying cause is the system's stdint.h is used first, which it should be avr-libc's stdint.h going first. (avr-g++ does so)

The patch fixes that issue, which add avr-libc to clang-avr toolchain's include path.

Thanks for your help!!

[dylanmckay]

Looks good, nice and simple. Thanks @benshi001

clang/lib/Driver/ToolChains/AVR.h

 namespace clang {
 namespace driver {
 namespace toolchains {
 
 class LLVM_LIBRARY_VISIBILITY AVRToolChain : public Generic_ELF {
 public:
   AVRToolChain(const Driver &D, const llvm::Triple &Triple,
                const llvm::opt::ArgList &Args);
+  void
+  AddClangSystemIncludeArgs(const llvm::opt::ArgList &DriverArgs,
+                            llvm::opt::ArgStringList &CC1Args) const override;
 
 protected:
   Tool *buildLinker() const override;
 
 private:
   /// Whether libgcc, libct, and friends should be linked.
   ///
   /// This is not done if the user does not specify a

clang/lib/Driver/ToolChains/AVR.cpp

if (CPU.empty()) {
       }
     }
 
     if (!LinkStdlib)
       D.Diag(diag::warn_drv_avr_stdlib_not_linked);
   }
 }
 
+void AVRToolChain::AddClangSystemIncludeArgs(const ArgList &DriverArgs,
+                                             ArgStringList &CC1Args) const {
+  if (DriverArgs.hasArg(options::OPT_nostdinc) ||
+      DriverArgs.hasArg(options::OPT_nostdlibinc))
+    return;
+
+  // Omit if there is no avr-libc installed.
+  Optional<std::string> AVRLibcRoot = findAVRLibcInstallation();
+  if (!AVRLibcRoot.hasValue())
+    return;
+
+  // Add 'avr-libc/include' to clang system include paths if applicable.
+  std::string AVRInc = AVRLibcRoot.getValue() + "/include";
+  if (llvm::sys::fs::is_directory(AVRInc))
+    addSystemInclude(DriverArgs, CC1Args, AVRInc);
+}
+
 Tool *AVRToolChain::buildLinker() const {
   return new tools::AVR::Linker(getTriple(), *this, LinkStdlib);
 }
 
 void AVR::Linker::ConstructJob(Compilation &C, const JobAction &JA,
                                const InputInfo &Output,
                                const InputInfoList &Inputs, const ArgList &Args,
                                const char *LinkingOutput) const {

clang/test/Driver/avr-toolchain.c

 // A basic clang -cc1 command-line.
 
 // RUN: %clang %s -### -no-canonical-prefixes -target avr 2>&1 | FileCheck -check-prefix=CC1 %s
 // CC1: clang{{.*}} "-cc1" "-triple" "avr"
+
+// RUN: %clang %s -### -target avr --sysroot %S/Inputs/basic_avr_tree 2>&1 | FileCheck -check-prefix CC1A %s
+// CC1A: clang{{.*}} "-cc1" "-triple" "avr" {{.*}} "-internal-isystem" {{".*avr/include"}}
+
+// RUN: %clang %s -### -target avr --sysroot %S/Inputs/basic_avr_tree 2>&1 -nostdinc | FileCheck -check-prefix CC1B %s
+// CC1B-NOT: "-internal-isystem" {{".*avr/include"}}
+
+// RUN: %clang %s -### -target avr --sysroot %S/Inputs/basic_avr_tree 2>&1 -nostdlibinc | FileCheck -check-prefix CC1C %s
+// CC1C-NOT: "-internal-isystem" {{".*avr/include"}}