This is an archive of the discontinued LLVM Phabricator instance.

[AVR] Automatically link CRT and libgcc from the system avr-gcc
ClosedPublic

Authored by dylanmckay on Nov 9 2018, 10:59 AM.

Details

Summary

This patch modifies the AVR toolchain so that if avr-gcc and avr-libc
are detected during compilation, the CRT, libgcc, libm, and libc anre
linked.

This matches avr-gcc's default behaviour, and the expected behaviour of
all C compilers - including the C runtime.

avr-gcc also needs a -mmcu specified in order to link runtime libraries.

The difference betwen this patch and avr-gcc is that this patch will
warn users whenever they compile without a runtime, as opposed to GCC,
which silently trims the runtime libs from the linker arguments when no
-mmcu is specified.

Diff Detail

Event Timeline

dylanmckay created this revision.Nov 9 2018, 10:59 AM

Add the search path that Ubuntu installs libc to.

I'd like feedback on the new AVR-specific warnings, and the new warning group. I have not added a warning or error to clang before, but this seems consistent with how the other targets implement it.

aaron.ballman added inline comments.Nov 16 2018, 5:14 AM
include/clang/Basic/DiagnosticDriverKinds.td
44–45

This is novel for diagnostics -- we don't have any other diagnostics that recommend filing bugs. This doesn't strike me as something a naive developer will just happen across by accident, so I would probably drop from the comma onward and assume the user knows they can report bugs if they really care.

lib/Driver/ToolChains/AVR.cpp
40

Why use std::string here when below you use it as a list of StringRefs?

92–94

Elide braces.

129

I'd drop this and inline it in the use below as *Family.

155–157

Elide braces.

160

return llvm::None;

lib/Driver/ToolChains/AVR.h
46–50

Did clang-format produce this? For some reason, it looks off to my eyes.

dylanmckay marked 6 inline comments as done.
  • Remove link to BugZilla in diagnostic
  • Use StringRef for a static string array rather than std::string
  • Elide braces
  • Dereference an Optional in-place rather than persisting it for one use
  • Use llvm::None rather than explicit Optional() constructor
dylanmckay marked an inline comment as done.

Run clang-format on the whole patch

dylanmckay added inline comments.Nov 16 2018, 6:00 AM
include/clang/Basic/DiagnosticDriverKinds.td
44–45

I agree

lib/Driver/ToolChains/AVR.cpp
40

Good point, have fixed.

160

Didn't know about that, thanks!

lib/Driver/ToolChains/AVR.h
46–50

I have now run clang-format on the whole patch.

I'm not certain if it will be possible to devise test cases for the two diagnostics I pointed out or not, but otherwise, this LGTM as far as the implementation goes. I don't know enough about AVR to sign off on whether the patch logic is correct or not.

include/clang/Basic/DiagnosticDriverKinds.td
35

I don't see a test case for this diagnostic.

40

Or this one.

dylanmckay marked 3 inline comments as done.

I'm not certain if it will be possible to devise test cases for the two diagnostics I pointed out or not

I don't think it will be. The compile warning logic works by directly querying the physical filesystem using the llvm::sys::fs APIs, which don't appear to support mocking or virtual filesystems. The only way for the pragmatically trigger the warnings is to dangerously manipulate the filesystem, potentially destroying the user's avr-gcc installation.

I don't know enough about AVR to sign off on whether the patch logic is correct or not.

I've added some review comments to the AVR logic referencing the GCC manual and how this patch matches it. I've also added the only two LLVM reviewers I know have AVR experience so hopefully they will be able to comment on the accuracy of the AVR logic.

lib/Driver/ToolChains/AVR.cpp
35

Here is the avr-gcc list of devices and their architectures

atmega328 and atmega328p are a part of the avr5 family.

avr5

“Enhanced” devices with 16 KiB up to 64 KiB of program memory.
mcu = ata5702m322, ata5782, ata5790, ata5790n, ata5791, ata5795, ata5831, ata6613c, ata6614q, ata8210, ata8510, atmega16, atmega16a, atmega16hva, atmega16hva2, atmega16hvb, atmega16hvbrevb, atmega16m1, atmega16u4, atmega161, atmega162, atmega163, atmega164a, atmega164p, atmega164pa, atmega165, atmega165a, atmega165p, atmega165pa, atmega168, atmega168a, atmega168p, atmega168pa, atmega168pb, atmega169, atmega169a, atmega169p, atmega169pa, atmega32, atmega32a, atmega32c1, atmega32hvb, atmega32hvbrevb, atmega32m1, atmega32u4, atmega32u6, atmega323, atmega324a, atmega324p, atmega324pa, atmega325, atmega325a, atmega325p, atmega325pa, atmega3250, atmega3250a, atmega3250p, atmega3250pa, atmega328, atmega328p, atmega328pb, atmega329, atmega329a, atmega329p, atmega329pa, atmega3290, atmega3290a, atmega3290p, atmega3290pa, atmega406, atmega64, atmega64a, atmega64c1, atmega64hve, atmega64hve2, atmega64m1, atmega64rfr2, atmega640, atmega644, atmega644a, atmega644p, atmega644pa, atmega644rfr2, atmega645, atmega645a, atmega645p, atmega6450, atmega6450a, atmega6450p, atmega649, atmega649a, atmega649p, atmega6490, atmega6490a, atmega6490p, at90can32, at90can64, at90pwm161, at90pwm216, at90pwm316, at90scr100, at90usb646, at90usb647, at94k, m3000.

https://gcc.gnu.org/onlinedocs/gcc/AVR-Options.html

139

Here is the avr-gcc manual documenting the filename of this particular libary[1].

Options:

-nodevicelib

Don’t link against AVR-LibC’s device specific library lib<mcu>.a.

This patch follows the same conventions as avr-gcc and thus can link the same libraries as avr-gcc.

[1] - https://gcc.gnu.org/onlinedocs/gcc/AVR-Options.html

143

This is an existing piece of avr-gcc logic also implemented by the LLVM AVR backend

-mmcu=mcu

Specify Atmel AVR instruction set architectures (ISA) or MCU type.

The default for this option is ‘avr2’.

https://gcc.gnu.org/onlinedocs/gcc/AVR-Options.html

I'm very comfortable with the AVR changes at this point, I am going to go ahead and commit the patch.

Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2019, 3:02 AM
This revision was not accepted when it landed; it landed in state Needs Review.May 19 2019, 2:54 AM
This revision was automatically updated to reflect the committed changes.