zapster marked an inline comment as done.
zapster added inline comments.
================
Comment at: llvm/lib/LTO/LTOBackend.cpp:335
+ .Case("bitcode", EmbedBitcodeKind::Embed_Bitcode)
+ .Case("marker", EmbedBitcodeKind::Embed_Marker)
+ .Default(~0U);
----------------
steven_wu wrote:
> 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.
>
>
> [...] 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
Right. I mixed up the case where the embedding logic was called from clang and
from LTO. And of course, the MacOS linker does (unfortunately) not use this
code.
> Presumably your tool doesn't use the marker section?
Nope, I don't need the marker section.
> I don't think marker is useful for this use case.
Right, makes sense. I'll update the review and make `-lto-embed-bitcode` a
boolean option.
Thanks to both of you for the clarifications.
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
