This is an archive of the discontinued LLVM Phabricator instance.

[Driver][AVR] Emit proper warnings for different options
ClosedPublic

Authored by benshi001 on Mar 26 2022, 12:39 AM.

Diff Detail

Event Timeline

benshi001 created this revision.Mar 26 2022, 12:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2022, 12:39 AM
benshi001 requested review of this revision.Mar 26 2022, 12:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2022, 12:39 AM
benshi001 updated this revision to Diff 418387.Mar 26 2022, 4:43 AM
  1. If '-S'/'-c' is specified, do not generate link warnings;
  2. If '-S'/'-c' is not specified, '-mmcu' is specified and there is valid GCC installation, do not generate link warnings;
  3. If '-S'/'-c' is not specified, and '-mmcu' is not specified, genereate link warnings;
  4. If '-S'/'-c' is not specified, and there is no valid GCC installation, genereate link warnings.
  1. If '-S'/'-c' is specified, do not generate link warnings;
    • User should be clear that he does not want link, so there is no link warning.
  1. If '-S'/'-c' is not specified, '-mmcu' is specified and there is valid GCC installation, do not generate link warnings;
    • This is the normal case, link should be succesful.
  1. If '-S'/'-c' is not specified, and '-mmcu' is not specified, genereate link warnings;
    • Since no MCU is specified, we should warn the user that no device library is linked.
  1. If '-S'/'-c' is not specified, and there is no valid avr-GCC installation, genereate link warnings.
    • Since there is no avr-ld installed, we should warn the user that there is no link happens. That may changes if we choose to use lld in the future.
benshi001 retitled this revision from [clang][AVR] Eliminate link warnings when '-S' is specified to [clang][AVR] Generate link warnings properly.Mar 26 2022, 5:18 AM
benshi001 edited the summary of this revision. (Show Details)
benshi001 edited the summary of this revision. (Show Details)
benshi001 edited the summary of this revision. (Show Details)Mar 26 2022, 5:22 AM
MaskRay added inline comments.Mar 27 2022, 10:37 AM
clang/test/Driver/avr-toolchain.c
39

-target is legacy spelling. Better use --target= for new RUN lines.

40

negative patterns easily go stale without being noticed.
Check whether you can just use CHECK5-NOT: warning: instead of specifying the full diagnostic.

MaskRay added inline comments.Mar 27 2022, 10:39 AM
clang/lib/Driver/ToolChains/AVR.cpp
378

This is insufficient. -fsyntax-only, -E, etc should not need the diagnostic as well.
I know there may be test coverage gap but it is excessive to test all of -S -c -E ... that there is no diagnostic.

benshi001 updated this revision to Diff 418481.Mar 27 2022, 7:16 PM
MaskRay added a comment.EditedMar 27 2022, 10:31 PM

I think it is excessive to add so many RUN lines. I do not understand much about AVR -mcpu. That said, I created D122553 for what I think should be done for the -c/-S/-fsyntax-only condition. More tests would just be excessive.

You may adjust the patch to do the rest cleanups/fixes.

benshi001 marked an inline comment as done.
benshi001 retitled this revision from [clang][AVR] Generate link warnings properly to [clang][AVR] Emit proper warnings.
benshi001 edited the summary of this revision. (Show Details)
benshi001 set the repository for this revision to rG LLVM Github Monorepo.

I think it is excessive to add so many RUN lines. I do not understand much about AVR -mcpu. That said, I created D122553 for what I think should be done for the -c/-S/-fsyntax-only condition. More tests would just be excessive.

You may adjust the patch to do the rest cleanups/fixes.

I have made some changes based on your https://reviews.llvm.org/D122553, it seems you have reverted. I suggest you recommit, since I have fixed the failures of lacking avr-gcc.

I think it is excessive to add so many RUN lines. I do not understand much about AVR -mcpu. That said, I created D122553 for what I think should be done for the -c/-S/-fsyntax-only condition. More tests would just be excessive.

You may adjust the patch to do the rest cleanups/fixes.

I have made some changes based on your https://reviews.llvm.org/D122553, it seems you have reverted. I suggest you recommit, since I have fixed the failures of lacking avr-gcc.

I did not specify --sysroot so the new tests failed on systems without avr-gcc. Relanded.

benshi001 updated this revision to Diff 418764.Mar 28 2022, 7:56 PM
benshi001 retitled this revision from [clang][AVR] Emit proper warnings to [Driver][AVR] Emit proper warnings for different options.

I think it is excessive to add so many RUN lines. I do not understand much about AVR -mcpu. That said, I created D122553 for what I think should be done for the -c/-S/-fsyntax-only condition. More tests would just be excessive.

You may adjust the patch to do the rest cleanups/fixes.

I have made some changes based on your https://reviews.llvm.org/D122553, it seems you have reverted. I suggest you recommit, since I have fixed the failures of lacking avr-gcc.

I did not specify --sysroot so the new tests failed on systems without avr-gcc. Relanded.

I have made some changes based on your code:

  1. We should always warn if there is no mmcu specified, since its info (instruction set version) is used by the llvm backend.
  2. We need no warn no avr-gcc&avr-libc if -S/-c is specified, since the compiling stage need neither avr-gcc nor avr-libc.

Thanks for your way to avoid checking so many -c/-S/-fsyntax-only/... conditions.

benshi001 marked 2 inline comments as done.Mar 28 2022, 8:02 PM
MaskRay accepted this revision.Mar 28 2022, 8:42 PM
MaskRay added inline comments.
clang/lib/Driver/ToolChains/AVR.cpp
451

The two if can be combined.

This revision is now accepted and ready to land.Mar 28 2022, 8:42 PM
benshi001 marked an inline comment as done.Mar 28 2022, 8:48 PM
benshi001 added inline comments.
clang/lib/Driver/ToolChains/AVR.cpp
451

I will do it when committing.

benshi001 marked an inline comment as done.Mar 28 2022, 8:55 PM
benshi001 added inline comments.
clang/lib/Driver/ToolChains/AVR.cpp
477

We should not merge the above two if, due to this check.

So I will keep my orginal form when committing.

MaskRay added inline comments.Mar 28 2022, 9:03 PM
clang/lib/Driver/ToolChains/AVR.cpp
477

OK. If we decide that empty -mpu will report a warning, reporting an additional warn_drv_avr_stdlib_not_linked may or may not be necessary, but I'm happy to defer that decision to you.

This revision was landed with ongoing or failed builds.Mar 28 2022, 9:05 PM
This revision was automatically updated to reflect the committed changes.