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

Is subtarget the best way to do it? #4457

Open
0dminnimda opened this issue Nov 5, 2024 · 6 comments
Open

Is subtarget the best way to do it? #4457

0dminnimda opened this issue Nov 5, 2024 · 6 comments

Comments

@0dminnimda
Copy link
Contributor

The way it's implemented is kind of messy. Instead of patching TargetMetrics

Odin/src/build_settings.cpp

Lines 1613 to 1624 in 1419d0d

if (metrics->os == TargetOs_darwin && subtarget == Subtarget_iOS) {
switch (metrics->arch) {
case TargetArch_arm64:
bc->metrics.target_triplet = str_lit("arm64-apple-ios");
break;
case TargetArch_amd64:
bc->metrics.target_triplet = str_lit("x86_64-apple-ios");
break;
default:
GB_PANIC("Unknown architecture for darwin");
}
}

Isn't it better to just list the variations explicitly in build_settings.cpp?

It's not too bad for iOS, but for android, the values differ not only for target triple, but also int_size and ptr_size, as well as there are things like i686-linux-android that don't have base target listed in build_settings.cpp.

And with explicit listing we could add the subtarget field to TargetMetrics and make choosing subtarget more turse and more natural.
-target:darwin_amd64 -subtarget:ios
->
-target:ios_amd64

And with explicit listing it's much more straight forward to allow the default TargetMetrics to be correct:

  1. Add checks for ios and android to src/gb/gb.h:
// GB_SYSTEM_OSX -> GB_SYSTEM_DARWIN 
#elif defined(__APPLE__) && defined(__MACH__)
    #ifndef GB_SYSTEM_DARWIN
    #define GB_SYSTEM_DARWIN 1
    #endif

    #include <TargetConditionals.h>
    #if TARGET_IPHONE_SIMULATOR == 1 || TARGET_OS_IPHONE == 1
    	#ifndef GB_SYSTEM_DARWIN_IOS
        #define GB_SYSTEM_DARWIN_IOS 1
        #endif
    #endif
#elif defined(__unix__)
	#if defined(__linux__)
		#if defined(__ANDROID__) || defined(__ANDROID_API__)
			#ifndef GB_SYSTEM_LINUX_ANDROID
			#define GB_SYSTEM_LINUX_ANDROID 1
			#endif
		#endif
  1. Choose the correct default metrics in init_build_context
	#if defined(GB_ARCH_64_BIT)
		#elif defined(GB_SYSTEM_DARWIN_IOS)
			#if defined(GB_CPU_ARM)
				metrics = &target_ios_arm64;
			#else
				metrics = &target_ios_amd64;
			#endif
		#elif defined(GB_SYSTEM_DARWIN)
			#if defined(GB_CPU_ARM)
				metrics = &target_darwin_arm64;
			#else
				metrics = &target_darwin_amd64;
			#endif
		#elif defined(GB_SYSTEM_LINUX_ANDROID)
			#if defined(GB_CPU_ARM)
				metrics = &target_android_arm64;
			#else
				metrics = &target_android_amd64;
			#endif
	#elif defined(GB_CPU_ARM)
		#elif defined(GB_SYSTEM_LINUX_ANDROID)
			metrics = &target_android_arm32;

And that's it!

All the metrics are alreay available in the build_context, so just change the selected_subtarget to build_context.metrics.subtarget


P.S. I understand where the name subtarget comes from, but I feel like it could be better called something along the lines of distro, but hey, the bike shed can be red!

@gingerBill
Copy link
Member

Yes this is the best way to do it. Subtargets like ios are much better than adding a completely new operating system to the batch. Darwin is the actual operating system and not macOS or iOS or whatever.

They share a lot of the same stuff any way and the main difference between them is mostly certain SDKs and linking changes.

Odin is very consistent in that it's os_arch for the target, where os is an OS/pseudo-OS/freestanding.

For Android I'd just have that be the a -subtarget for Linux, e.g. -subtarget:android.

@0dminnimda
Copy link
Contributor Author

I omited some details, sorry for that. I don't want to mark them as a diffrent OS, on a contrary I'm quite opposed to it. The change will not affect user code, only the internal handling in the compiler and the flags

So the build_settings.cpp will look something like

gb_global TargetMetrics target_darwin_amd64 = {
	TargetOs_darwin,
	TargetArch_amd64,
	8, 8, AMD64_MAX_ALIGNMENT, 32,
	str_lit("x86_64-apple-macosx"),
};
gb_global TargetMetrics target_darwin_arm64 = {
	TargetOs_darwin,
	TargetArch_arm64,
	8, 8, 16, 32,
	str_lit("arm64-apple-macosx"),
};
gb_global TargetMetrics target_ios_amd64 = {
	TargetOs_darwin,
	TargetArch_amd64,
	8, 8, AMD64_MAX_ALIGNMENT, 32,
	str_lit("x86_64-apple-ios"),
	Subtarget_iOS,
};
gb_global TargetMetrics target_ios_arm64 = {
	TargetOs_darwin,
	TargetArch_arm64,
	8, 8, 16, 32,
	str_lit("arm64-apple-ios"),
	Subtarget_iOS,
};

@0dminnimda
Copy link
Contributor Author

Doesn't ios implies Darwin? It does. Doesn't android implies Linux? It does. Does it makes sense to write -target:windows_amd64 -subtarget:ios? No. It's good to eliminate such possibly.

If you don't like ios_arm64, it could be named darwin_ios_arm64, it would not change the idea.

@gingerBill
Copy link
Member

You are correct that this doesn't make sense: -target:windows_amd64 -subtarget:ios

But it's literally a subtarget and the compiler will literally check for valid combinations. js_i386 makes no sense too but that's just a normal target.

@0dminnimda
Copy link
Contributor Author

So you're saying that the cli backward compatibility is more important here. That's reasonable.

Now though what's the intention for making it a separate flag? Why not have a separate os and arch then? I don't understand.

@0dminnimda
Copy link
Contributor Author

0dminnimda commented Nov 6, 2024

cli flags don't bother me a lot, but the fact that currently subtargets don't get default values (unlike metrics), also that you cannot specify a subtarget without a target and the fact that subtarget does an ugly patch for the metrics. Those do bother me. You must elaborate on those points.

It's silly to be required to always run odin with -target:<why_should_i_bother_to_know_this_as_a_user> -subtarget:android. The compiler should know what platform it's compiled for and be able to work on it without a reminder. Imagine having who always specify -target:windows_amd64, even though you are running on that platform.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants