odin-lang / Odin

Odin Programming Language
https://odin-lang.org
BSD 3-Clause "New" or "Revised" License
7.01k stars 621 forks source link

Is `subtarget` the best way to do it? #4457

Open 0dminnimda opened 2 weeks ago

0dminnimda commented 2 weeks ago

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

https://github.com/odin-lang/Odin/blob/1419d0d295919a625ff75dfa9c3ffc6a4c7efc8b/src/build_settings.cpp#L1613-L1624

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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.

gingerBill commented 2 weeks ago

@0dminnimda

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

Not even just that. They are bundled together so there is no reason to separate them. The subtarget is just that, subject to the target; it's optional and doesn't change the fundamental "platform target" that is being used.