Page MenuHomePhabricator

[clang] Limit bitcode option ignorelist to Darwin
AcceptedPublic

Authored by MichielDerhaeg on May 19 2022, 7:23 AM.

Details

Summary

This ignore list prevents embedding of bitcode for some platforms
without jumping through some hoops as several of the flags are passed by default.
E.g. On Android, since NDK 21, -ffunction-sections, -fdata-sections,
-mstackrealign, etc.. are added by default.

The ignore list is limited to Apple-based platforms.
Flags that are not expressed in some way in bitcode are now also passed to ensure they take effect.

Diff Detail

Event Timeline

MichielDerhaeg created this revision.May 19 2022, 7:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 7:23 AM
MichielDerhaeg requested review of this revision.May 19 2022, 7:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 19 2022, 7:23 AM
MichielDerhaeg retitled this revision from Limit bitcode option ignorelist to Darwin to [clang] Limit bitcode option ignorelist to Darwin.May 19 2022, 7:27 AM
MichielDerhaeg added a subscriber: wanders.

LGTM. Can you add a test for your usecase?

wanders added inline comments.May 19 2022, 1:16 PM
clang/lib/Driver/ToolChains/Clang.cpp
4759–4760

This comment and variable name are not as accurate any longer. Maybe moving it inside if (RawTriple.isOSDarwin()) makes it clearer.

  • Render options for other platforms and test
  • comment and propagate list
MichielDerhaeg marked an inline comment as done.May 23 2022, 2:15 PM
MichielDerhaeg added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
4759–4760

Changed the comment to what I could derive from D61627: [clang driver] Allow -fembed-bitcode combined with -mno-red-zone.

MichielDerhaeg edited the summary of this revision. (Show Details)May 23 2022, 2:17 PM

This looks sensible to me. But I'm not an expert in the area (and still not 100% clear on the purpose of the ignorelist) so can't really say if the added comments captures the intent accurately.

clang/test/Driver/embed-bitcode-elf.c
2

Should this test have a // REQUIRES: aarch64-registered-target?

MichielDerhaeg marked an inline comment as done.
  • add target requirement to lit test
  • move condition
MichielDerhaeg marked an inline comment as done.May 24 2022, 1:02 PM
steven_wu accepted this revision.May 26 2022, 2:22 PM

Thanks! LGTM.

This revision is now accepted and ready to land.May 26 2022, 2:22 PM