Page MenuHomePhabricator

[AVR][clang] Improve search for avr-libc installation path
ClosedPublic

Authored by benshi001 on Aug 6 2021, 8:41 PM.

Details

Summary

Search avr-libc path according to avr-gcc installation at first,
then other possible installed pathes.

Diff Detail

Event Timeline

benshi001 created this revision.Aug 6 2021, 8:41 PM
benshi001 requested review of this revision.Aug 6 2021, 8:41 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 6 2021, 8:41 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

There are already test cases

$llvm/clang/test/Driver/avr-ld.c
$llvm/clang/test/Driveravr-toolchain.c

and fake avr-gcc inside $llvm/clang/test/Driver/Inputs/basic_avr_tree to support the above two tests.

So there is no need to add new ones.

benshi001 updated this revision to Diff 364938.Aug 7 2021, 12:01 AM
benshi001 retitled this revision from [AVR][clang] Search avr-libc installtion path according to avr-gcc's to [AVR][clang] Improve search for avr-libc installation path.
benshi001 edited the summary of this revision. (Show Details)
mhjacobson added inline comments.Aug 7 2021, 12:08 AM
clang/lib/Driver/ToolChains/AVR.cpp
451

Would it be worth encapsulating this logic in an accessor method on GCCInstallation?

benshi001 updated this revision to Diff 364940.Aug 7 2021, 1:17 AM
benshi001 marked an inline comment as done.Aug 7 2021, 1:22 AM

Thanks. I have added some comments about our agreed logic, as discussed in https://reviews.llvm.org/D107672.

I think this way is clear enough.

@mhjacobson Could you please check if my modification also works as expected on your platform ?

mhjacobson added a comment.EditedAug 7 2021, 1:43 AM

@mhjacobson Could you please check if my modification also works as expected on your platform ?

Hm... it doesn't, because there aren't enough ..s.

Here's what my directory tree looks like. My gcc install consists of:

/opt/local/bin/avr-gcc-10.3.0
/opt/local/lib/gcc/avr/10.3.0/libgcc.a
/opt/local/libexec/gcc/avr/10.3.0/cc1

And avr-libc is at:

/opt/local/avr/lib/libc.a
/opt/local/avr/include/stdio.h

It would work if you appended four ..s (which is effectively what GCC does, since it backs up to the "prefix", which in my case is /opt/local/).

Edit: my setup seems to match the one shown in this presentation (see slides titled "Toolchain contents").

Edit 2: it also matches this, namely /opt/local/avr/lib is $(tooldir)/lib

For other examples of using four ..s, look in Gnu.cpp, where there are several places where getParentLibPath() is appended with /../. AddMultilibPaths(), PushPPaths().

getParentLibPath(), in turn, is GCCInstallPath/../../../

benshi001 updated this revision to Diff 364954.Aug 7 2021, 5:15 AM

For other examples of using four ..s, look in Gnu.cpp, where there are several places where getParentLibPath() is appended with /../. AddMultilibPaths(), PushPPaths().

getParentLibPath(), in turn, is GCCInstallPath/../../../

Thanks for your help. I have updated my patch. Now it should work for both your platform and mine. It search GCCRoot.getParentLibPath()/avr first, otherwise GCCRoot.getParentLibPath()/../avr.

Please help me check.

benshi001 updated this revision to Diff 364955.Aug 7 2021, 5:43 AM
benshi001 edited the summary of this revision. (Show Details)
MaskRay added inline comments.Aug 8 2021, 9:58 AM
clang/lib/Driver/ToolChains/AVR.cpp
454

return Path;

clang/test/Driver/avr-toolchain.c
1 ↗(On Diff #364955)

You can UNSUPPORT windows if you don't want to wrestle with \ and / path separators.

1 ↗(On Diff #364955)

UNSUPPORTED: system-windows

benshi001 updated this revision to Diff 365089.Aug 8 2021, 11:16 PM
benshi001 marked 3 inline comments as done.

For other examples of using four ..s, look in Gnu.cpp, where there are several places where getParentLibPath() is appended with /../. AddMultilibPaths(), PushPPaths().

getParentLibPath(), in turn, is GCCInstallPath/../../../

Thanks for your help. I have updated my patch. Now it should work for both your platform and mine. It search GCCRoot.getParentLibPath()/avr first, otherwise GCCRoot.getParentLibPath()/../avr.

Please help me check.

Latest diff works for me, thanks!

benshi001 updated this revision to Diff 365347.Aug 9 2021, 9:14 PM

@MaskRay

I have update my patch based on your new test avr-toolchain.c.

Thank you!

Seems a new RUN line is needed. If having too many mock trees are inconvenient, consider clang/unittests/Driver/ToolChainTest.cpp

benshi001 updated this revision to Diff 365996.Aug 12 2021, 7:48 AM

Seems a new RUN line is needed. If having too many mock trees are inconvenient, consider clang/unittests/Driver/ToolChainTest.cpp

I am not familiar with the way in clang/unittests/Driver/ToolChainTest.cpp, I always use clang command line than libcall. Though I have created a new test TEST(ToolChainTest, VFSAVRInstallation) by imitating the existing TEST(ToolChainTest, VFSGCCInstallation), it did not work as I expected. So I created a new mock tree basic_avr_tree_2.

And the tests in avr-toolchain.cc should cover all three possible cases

  1. find avr-libc at AVR-GCC/../../../avr (the case --check-prefix=CHECK1)
  2. find avr-libc at AVR-GCC/../../../../avr (the case --check-prefix=CHECK2)
  3. there is not avr-gcc and find avr-libc at SYSROOT/usr/avr (the case --check-prefix=CHECK3)
benshi001 added a comment.EditedAug 12 2021, 7:58 AM

The file/directory structure in basic_avr_tree_2/opt/local is the same as mhjacobson's machine.

basic_avr_tree_2/opt/local/bin/avr-gcc-10.3.0
basic_avr_tree_2/opt/local/lib/gcc/avr/10.3.0/libgcc.a

basic_avr_tree_2/opt/local/avr/
basic_avr_tree_2/opt/local/avr/lib/libc.a
basic_avr_tree_2/opt/local/avr/include/.keep

The above is for CHECK2, and there is an avr-libc only structure for CHECK3.

basic_avr_tree_2/usr/avr/
basic_avr_tree_2/usr/avr/lib/libc.a
basic_avr_tree_2/usr/avr/include/.keep
MaskRay accepted this revision.Aug 16 2021, 8:46 PM
This revision is now accepted and ready to land.Aug 16 2021, 8:46 PM