steven_wu added inline comments.
================
Comment at: llvm/lib/LTO/LTOBackend.cpp:335
+ .Case("bitcode", EmbedBitcodeKind::Embed_Bitcode)
+ .Case("marker", EmbedBitcodeKind::Embed_Marker)
+ .Default(~0U);
----------------
tejohnson wrote:
> zapster wrote:
> > tejohnson wrote:
> > > Does this value make any sense since CmdArgs is always empty below?
> > You are right. Currently, this option value is not utterly useful, but I
> > kept it for compatibility with clangs `-fembed-bitcode`. It seems the
> > marker section is needed (even if empty), otherwise certain tools will
> > complain (notably the MacOS linker). Ideally, we would have something
> > sensible to write into the section but I am not sure about reasonable
> > values. I was not able to find any information about consumers of these
> > commands. Pointers in that regard would be highly appreciated.
> >
> > Anyhow, I lean toward keeping the options for compatibility with clang and
> > for making it simple to properly implement the `marker` case. That said, I
> > am perfectly fine with removing the option. In that case, I guess I should
> > get rid of `EmbedBitcodeKind` altogether and always insert the bitcode and
> > the marker, right?
> >
> > On the other hand, we should maybe just consolidate the option (parsing)
> > with the clang option. Some thoughts about that in my inline comment above.
> It's unclear to me whether it is better to not provide the marker, and get a
> clear error, or put in an empty marker which may cause these tools to do the
> wrong thing. @steven_wu who might be able to provide some guidance about how
> this is used by the MacOS linker. Note that if someone is using MacOS to do
> their LTO linking, they won't even trigger this code, as that toolchain uses
> the old legacy LTO, which doesn't come here.
>
> Presumably your tool doesn't use the marker section?
I don't think marker is useful for this use case. I will suggest not to have
the `marker` LTO option unless we have the need. It is being tested by clang
and that is probably enough coverage for testing.
I don't think any users of the legacy interface express interests in using a
unified bitcode section so it is fine just to leave this out of the legacy C
interface.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D68213/new/
https://reviews.llvm.org/D68213
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
