[llvm] r315854 - Reverting r315590; it did not include changes for llvm-tblgen, which is causing link errors for several people.

[Justin Bogner via llvm-commits]

Don Hinton <hintonda at gmail.com> writes:
> Okay, I'll re-open the diff and post a new patch with only the cmake
> changes.  Then we can iterate over any changes.
>
> On Mon, Dec 4, 2017 at 5:37 PM, Justin Bogner <mail at justinbogner.com> wrote:
>
>> Don Hinton <hintonda at gmail.com> writes:
>> > Hi Justin:
>> >
>> > The problem was with generators like XCode and MSVC that create multiple
>> > configurations, e.g., Release, Debug, etc., and need to rely on passing
>> > -DNDEBUG in order to work.  That makes it tough to use a variable like
>> > LLVM_ENABLE_DUMP that gets set at configure time..
>>
>> One other option may be to pass the -DLLVM_ENABLE_DUMP flag to the build
>> as well as adding it to llvm-config.h. Would that work?
>
> Sure, I can do that.
>
>>> We could probably do a partial change that doesn't remove the
>>> NDEBUG tests, but it still wouldn't work well with XCode and MSVC
>>> generators -- would still compile and link, but just not as useful
>>> on a per configuration basis.
>>>
>>> Please let me know if that would be helpful, and I'll create a new
>>> diff for you to review.
>>
>> At the very least we need to make the change to move the
>> LLVM_ENABLE_DUMP definition into Config/llvm-config.h. Headers aren't
>> allowed to include Config/config.h (since it isn't installed), which
>> means that currently we have some uses that aren't actually correct.
>>
>> An example of this is MC/MCSchedule.h, where we check LLVM_ENABLE_DUMP,
>> but we don't include anything that defines it.
>
> To support this case, we'd have to pass it on the comandline, not in
> llvm-config.h.

I disagree. The correct way to support this case is to put it in
llvm-config.h and include llvm-config.h in MCSchedule.h.

This way non-llvm code that includes MCSchedule.h will get the correct
definitions. If we just pass it on the command line those can and will
get it wrong.

>> I think we should break this into two pieces: 1. Move LLVM_ENABLE_DUMP
>> to llvm-config.h, and 2. Try to make this work with multi-config
>> generators.
>
> Now that you mention it, I think we could add -DLLVM_ENABLE_DUMP to
> the various CMAKE_CXX_FLAGS_${CMAKE_BUILD_TYPE} variables that are
> configuration specific instead of putting it in config.h or
> llvm-config.h.
>
> That would solve the MC/MCSchedule.h issue.

This doesn't sound right to me. If we don't put this in llvm-config.h
we'll have the same problem we do now where any projects including llvm
headers will run into issues with getting the defines right.

> Still not sure about how to effectively use XCode and MSVC generators,
> but I guess the user clicks on something to change things once they
> are generated.  So generating defaults should be fine.
>
>
>> > thanks...
>> > don
>> >
>> >
>> >
>> > On Mon, Dec 4, 2017 at 4:55 PM, Justin Bogner <mail at justinbogner.com>
>> wrote:
>> >
>> >> Aaron Ballman <aaron at aaronballman.com> writes:
>> >> > On Mon, Dec 4, 2017 at 5:24 PM, Justin Bogner <mail at justinbogner.com>
>> >> wrote:
>> >> >> Aaron Ballman via llvm-commits <llvm-commits at lists.llvm.org> writes:
>> >> >>> Author: aaronballman
>> >> >>> Date: Sun Oct 15 07:32:27 2017
>> >> >>> New Revision: 315854
>> >> >>>
>> >> >>> URL: http://llvm.org/viewvc/llvm-project?rev=315854&view=rev
>> >> >>> Log:
>> >> >>> Reverting r315590; it did not include changes for llvm-tblgen, which
>> >> >>> is causing link errors for several people.
>> >> >>
>> >> >> What's happening with this? Is someone investigating these errors?
>> I'd
>> >> >> really like the part of this change that moves LLVM_ENABLE_DUMP into
>> >> >> llvm-config.h to get in, as it's currently kind of awkward to use in
>> the
>> >> >> half finished state.
>> >> >>
>> >> >>> Error LNK2019 unresolved external symbol "public: void __cdecl
>> >> >>> `anonymous namespace'::MatchableInfo::dump(void)const "
>> >> >>> (?dump at MatchableInfo@?A0xf4f1c304@@QEBAXXZ) referenced in function
>> >> >>> "public: void __cdecl `anonymous
>> >> >>> namespace'::AsmMatcherEmitter::run(class llvm::raw_ostream &)"
>> >> >>> (?run at AsmMatcherEmitter@?A0xf4f1c304@@QEAAXAEAVraw_ostream at llvm
>> @@@Z)
>> >> >>> llvm-tblgen D:\llvm\2017\utils\TableGen\AsmMatcherEmitter.obj 1
>> >> >
>> >> > I've not been investigating -- I was only trying to get the bot back
>> >> > to green. Perhaps Don has worked on this, however.
>> >>
>> >> Okay, I couldn't find any email trail of followup (not even to mention
>> >> the commit was reverted!), so I wasn't sure if I'd just missed the
>> >> conversation here.
>> >>
>>