Page MenuHomePhabricator

[clang][Fuchsia] Support __attribute__((availability)) on Fuchsia
ClosedPublic

Authored by haowei on Aug 23 2021, 3:41 PM.

Details

Summary

Also in this patch:

  • Control the fuchsia version via a new flag (-ffuchsia-api-level).
  • Add a warning that triggers when a non-zero minor/subminor are used for the fuchsia version.

Diff Detail

Event Timeline

leonardchan created this revision.Aug 23 2021, 3:41 PM
leonardchan requested review of this revision.Aug 23 2021, 3:41 PM
leonardchan planned changes to this revision.Aug 23 2021, 3:43 PM

Some extra comments/open questions:

  • This patch supports using N.0.0 as a version number. Do we want to just only accept N and disallow any number for minor/subminor, or will zeros be allowed?
  • This currently will only work if the major number is provided via the target triple (x86_64-unknown-fuchsia16). Do we have a concept of a default version number for fuchsia we can fall back to if this is not provided?
leonardchan edited the summary of this revision. (Show Details)
leonardchan retitled this revision from [WIP][clang][Fuchsia] Support __attribute__((availability)) on Fuchsia to [clang][Fuchsia] Support __attribute__((availability)) on Fuchsia.Aug 24 2021, 12:04 PM
phosek added inline comments.Sep 9 2021, 10:59 AM
clang/include/clang/Basic/LangOptions.def
434

This is more consistent with other options.

clang/lib/Basic/Targets/OSTargets.h
893

We also want to define a __FUCHSIA_VERSION__ so the version can be read at compile time.

clang/test/Frontend/attr-availability-fuchsia.c
4 ↗(On Diff #368423)

Would this also set the availability attribute? Can we test it? If not then this isn't particularly useful.

aaron.ballman added inline comments.Sep 9 2021, 11:49 AM
clang/test/Frontend/attr-availability-fuchsia.c
2 ↗(On Diff #368423)

Is |& intentional? I think that's causing shell parse errors on Windows (same for the other test).

Also should this test be in Driver instead as it's testing the driver functionality?

haowei commandeered this revision.Tue, Oct 5, 4:50 PM
haowei added a reviewer: leonardchan.
haowei added a subscriber: haowei.

I am commandeering this work as original author stopped working this project.

haowei updated this revision to Diff 377389.Tue, Oct 5, 4:51 PM
haowei edited the summary of this revision. (Show Details)
aaron.ballman added inline comments.Wed, Oct 6, 4:45 AM
clang/include/clang/Basic/DiagnosticSemaKinds.td
3558
clang/lib/Basic/Targets/OSTargets.h
893

I think this macro should be using a reserved name, as suggested by @phosek.

clang/test/Frontend/attr-availability-fuchsia.c
2 ↗(On Diff #377389)

Can you also add a test that shows how we handle -ffuchsia-api-level=16.0.0?

2 ↗(On Diff #368423)

This test should be in Driver, right?

clang/test/Sema/attr-availability-fuchsia.c
15

FWIW, this surprises me. I would have expected that 16, 16.0, and 16.0.0 were all equivalent and the issue was with specifying a nonzero value for those. I've suggested new wording for the diagnostic that might make it more clear.

phosek added inline comments.Wed, Oct 6, 12:20 PM
clang/lib/Basic/Targets/OSTargets.h
893

@aaron.ballman Do you have any thoughts on the letter case? I think that __Fuchsia_API_level__ might be a better match for __Fuchsia__. Upper case is perhaps more usual, but other platforms like *BSD also use mixed case.

haowei updated this revision to Diff 377726.Wed, Oct 6, 5:48 PM
haowei marked an inline comment as done.
haowei marked an inline comment as done.Wed, Oct 6, 5:51 PM
haowei added inline comments.
clang/lib/Basic/Targets/OSTargets.h
893

I changed it to "Fuchsia_API_level" for now.

clang/test/Frontend/attr-availability-fuchsia.c
2 ↗(On Diff #377389)

I did. But it does not fail. I will look into it to see if I can add the same constraint like the attributes tomorrow.

4 ↗(On Diff #368423)

Do you mean the macros? I can add one.

aaron.ballman added inline comments.Thu, Oct 7, 8:40 AM
clang/lib/Basic/Targets/OSTargets.h
893

I don't have strong opinions so long as the identifier is reserved (and it is). I would note that the current capitalization might be a bit annoying for users to remember because it's Fuchsia, API, and level, all with different casing styles.

haowei updated this revision to Diff 378052.Thu, Oct 7, 5:51 PM

I added unit test for macros. I am still working on adding error checks on --ffuchsia-api-level flags but the rest is pretty much done.

clang/test/Frontend/attr-availability-fuchsia.c
4 ↗(On Diff #368423)

@phosek I added unit test for macros. Could you confirm it is what you suggested in the previous comments? Thanks.

haowei updated this revision to Diff 378324.Fri, Oct 8, 11:49 AM

Added unit test when "-ffuchsia-api-level" is supplied with a non integer value.

phosek accepted this revision.Fri, Oct 8, 2:20 PM

LGTM

clang/include/clang/Basic/LangOptions.def
434
clang/include/clang/Driver/Options.td
3177
clang/test/Driver/attr-availability-fuchsia.c
3–26

Can you reorder this so CHECK always follows the corresponding RUN and separate each case by an empty line? I find it more readable.

This revision is now accepted and ready to land.Fri, Oct 8, 2:20 PM
haowei updated this revision to Diff 378780.Mon, Oct 11, 1:22 PM

Fix test failures on Windows.

haowei updated this revision to Diff 378784.Mon, Oct 11, 1:41 PM

Address review comments.

This revision was automatically updated to reflect the committed changes.