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

Fix TI C28x (C2000, etc.) support #13681

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

EngJay
Copy link

@EngJay EngJay commented Sep 15, 2024

Summary

The existing support for TI C28x cross compilation (C2000, C6000, etc.) produces the linker rule with the arguments in the incorrect order and omits the compiler args that are required to also be included in the linker command for TI C28x. An example that demonstrates the issue has been created as a repo with a Docker image.

An exception has been added to the generate_dynamic_link_rules() method of the Ninja backend to produce the rule with the args in the correct order for this linker. To populate the variables for the rule, an exception has also been added to the generate_link() method.

This is not the ideal way to do this but it works. Ideally, compilers and linkers would provide their own ordering of arguments to avoid hard-coding the order in the Ninja backend but I sought to fix the TI C28x support with minimal changes. A more durable solution would be to move the ordering of the elements of commands to the compiler and linker classes, so it gets injected into the generation of the rules.

  • resolves #13768

Details

Generating the correct rule format for TI C28x linking

To produce the linking rule for TI C28x with the correct form, the generate_dynamic_link_rules() method of the NinjaBackend class has been modified to switch forms when the linker ID is cl2000, cl6000, or ti.

Original.

args = ['$ARGS'] + NinjaCommandArg.list(compiler.get_linker_output_args('$out'), Quoting.none) + ['$in', '$LINK_ARGS']

Exception added.

if compiler.get_linker_id() in {'cl2000', 'cl6000', 'ti'}:
    # TI C28x linker requires a different order of args and
    # static libs last with the full name.
    args += ['$LINK_ARGS', '$in', '$TI_C28X_STATIC_LIBS']
else:
    args += ['$in', '$LINK_ARGS']

This exception produces the correct linker rule format for TI C28x.

rule c_LINKER
 command = cl2000 $ARGS -z --output_file=$out $LINK_ARGS $in $TI_C28X_STATIC_LIBS
 description = Linking target $out

This change, however, caused a bunch test failures because the StaticLinker class did not have a get_id() method despite having an id, so a method has been added that matches the DynamicLinker class's method.

Populating the $ARGS and $TI_C28X_STATIC_LIBS Ninja args in linking

I spent some time attempting to figure out how to get the compiler args into the generate_link() method but resorted to using the guts from the generate_compile_rules() method in generate_link(). I imagine there is a better way to do this but this got it working and I can go from here.

Addition of method to get ID of static linkers

The changes initially caused many test failures due to the lack of the same method for getting the ID of dynamic linkers on the static linker base class. The same method, get_id(), has now been implemented on the StaticLinker class, too, so the method can be used to identify the linker in places where the instance can be either type.

Testing

All tests pass locally except a couple that appear related to my environment. I did not find unit tests for the Ninja backend methods but the CI is passing and I have verified this works successfully with a minimal cross build with C2000. I could provide a self-contained example as a Docker image, if needed, so the minimal example can be run by a reviewer.

@EngJay EngJay marked this pull request as ready for review September 16, 2024 22:03
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

Successfully merging this pull request may close these issues.

1 participant