Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistency in macos/macosx version CLI flag vs. availability attributes/macros #86376

Closed
h-vetinari opened this issue Mar 23, 2024 · 6 comments · Fixed by #95374
Closed

Inconsistency in macos/macosx version CLI flag vs. availability attributes/macros #86376

h-vetinari opened this issue Mar 23, 2024 · 6 comments · Fixed by #95374
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl'

Comments

@h-vetinari
Copy link
Contributor

The clang attribute reference says:

  • macos:
    Apple’s macOS operating system. The minimum deployment target is specified by the -mmacosx-version-min=*version* command-line argument. macosx is supported for backward-compatibility reasons, but it is deprecated.

So we learn that the attribute should be spelled with macos, not macosx:

__attribute__((availability(macos,introduced=10.14)))

however, the CLI-flag as documented still uses the "x":

-mmacosx-version-min=*version*
       ^
       X

At the very least, this leads to confusion, as both forms are found in the current LLVM repo:

I also believe it may lead to bugs, in the sense that -mmacosx-version-min=10.9 in newer clang ends up not setting MAC_OS_X_VERSION_MIN_REQUIRED or __MAC_OS_X_VERSION_MIN_REQUIRED?

I don't have a mac to test this hypothesis, but this is consistent with the failures I'm seeing.

@EugeneZelenko EugeneZelenko added clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' and removed new issue labels Mar 23, 2024
@llvmbot
Copy link
Member

llvmbot commented Mar 23, 2024

@llvm/issue-subscribers-clang-driver

Author: None (h-vetinari)

The clang attribute [reference](https://clang.llvm.org/docs/AttributeReference.html#availability) says: > * `macos`: > Apple’s macOS operating system. The minimum deployment target is specified by the `-mmacosx-version-min=*version*` command-line argument. `macosx` is supported for backward-compatibility reasons, but it is deprecated.

So we learn that the attribute should be spelled with macos, not macosx:

__attribute__((availability(macos,introduced=10.14)))

however, the CLI-flag as documented still uses the "x":

-mmacosx-version-min=*version*
       ^
       X

At the very least, this leads to confusion, as both forms are found in the current LLVM repo:

I also believe it may lead to bugs, in the sense that -mmacosx-version-min=10.9 in newer clang ends up not setting MAC_OS_X_VERSION_MIN_REQUIRED or __MAC_OS_X_VERSION_MIN_REQUIRED?

I don't have a mac to test this hypothesis, but this is consistent with the failures I'm seeing.

@h-vetinari
Copy link
Contributor Author

CC @ldionne

@Xazax-hun
Copy link
Collaborator

The driver currently accepts both -mmacosx-version-min=10.9 and -mmacos-version-min=10.9, they are aliases:

def : Joined<["-"], "mmacosx-version-min=">,

I tested them with the following small program and both seem to work as expected:

#include <AvailabilityMacros.h>
#include <iostream>

int main() {
  std::cout << MAC_OS_X_VERSION_MIN_REQUIRED << " "  << __MAC_OS_X_VERSION_MIN_REQUIRED << std::endl;
}

Is there any action item in this ticket? E.g., do we want to make one of the flags deprecated, potentially emitting a warning to migrate to another one? I am not sure how often are we doing that in Clang.

@h-vetinari
Copy link
Contributor Author

Thanks for the response!

From my POV, the default CLI flag should match the naming in the availability macros, i.e. use macos not macosx. I don't care particularly if or how long the macosx spelling is kept around as an alias, but aside from the definition of the alias (and perhaps some docs about the preferred way going forward), all mentions of -mmacosx-version-min in the codebase should vanish IMO (and replaced by -mmacos-version-min).

@ldionne
Copy link
Member

ldionne commented Jun 12, 2024

^ I agree. Basically, the action item in this ticket would be to make sure that the default flag is -mmacos-version-min and that the other one is an alias for it, and to update any references in the documentation that use -mmacosx-version-min to start steering users towards the preferred spelling.

Xazax-hun pushed a commit to Xazax-hun/llvm-project that referenced this issue Jun 13, 2024
The -mmacos-version-min flag is preferred over -mmacosx-version-min. This patch
makes updates the tests and documentation to make this clear and also adds the
missing logic to scan build to handle the new flag.

Fixes llvm#86376.
@Xazax-hun
Copy link
Collaborator

I opened a PR to update the documentation and tests. I also realized scan-build did not properly handle the new spelling, so I also updated that.

Xazax-hun pushed a commit to Xazax-hun/llvm-project that referenced this issue Jun 16, 2024
The -mmacos-version-min flag is preferred over -mmacosx-version-min. This patch
makes updates the tests and documentation to make this clear and also adds the
missing logic to scan build to handle the new flag.

Fixes llvm#86376.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl'
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants