Page MenuHomePhabricator

[compiler-rt] Allow appending to 'target_cflags' value from lit_config.
ClosedPublic

Authored by abidh on Nov 19 2020, 4:34 AM.

Details

Summary

This patch is similar to D84708. When testing compiler-rt on different
baremetal targets, it helps to have the ability to pass some more parameters
at test time that allows you to build the test executable for a
given target. For an example, you may need a different linker command
file for different targets.

This patch will allows to do things like

$ llvm-lit --param=append_target_cflags="-T simulator.ld"
or
$ llvm-lit --param=append_target_cflags="-T hardware.ld"

In this way, you can run tests on different targets without having to run
cmake again.

Diff Detail

Event Timeline

abidh created this revision.Nov 19 2020, 4:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 19 2020, 4:34 AM
Herald added subscribers: Restricted Project, dberris. · View Herald Transcript
abidh requested review of this revision.Nov 19 2020, 4:34 AM
delcypher requested changes to this revision.Dec 6 2020, 4:32 PM

This seems reasonable but I do have a few nits.

In general though you probably be better off moving to an approach where you can CMake generate all the configs you want. I realise this might fit your workflow right now so I'm happy to land this patch once we address the nits.

compiler-rt/test/lit.common.cfg.py
581

Nit: Given that this appends rather than replaces you might want to change the name of target_cflags to something like append_target_cflags to avoid confusion.

582

You could add something to print out a message when the append happens. E.g.:

if lit_config_cflags:
  lit_config.note('Appending to extra_cflags: "{}:"'.format(lit_config_cflags))
  extra_cflags += [lit_config_cflags]
583

Why do you append to extra_cflags here? There appear to be other statements after this that append to extra_cflags. My expectation would be the using --param=append_target_cflags="-T simulator.ld" would cause the flags to appended to the very end. My suggestion would be change your code so that it is the very last append to extra_cflags.

This revision now requires changes to proceed.Dec 6 2020, 4:32 PM
abidh updated this revision to Diff 310018.Dec 7 2020, 2:04 PM
abidh edited the summary of this revision. (Show Details)

Addressed review comments.

abidh marked 3 inline comments as done.Dec 7 2020, 2:04 PM
delcypher accepted this revision.Dec 7 2020, 4:57 PM

Thanks for addressing my feedback. LGTM

This revision is now accepted and ready to land.Dec 7 2020, 4:57 PM