[llvm-commits] [llvm] r172630 - in /llvm/trunk: include/llvm/MC/MCContext.h lib/MC/MCDwarf.cpp test/MC/MachO/gen-dwarf-producer.s tools/llvm-mc/llvm-mc.cpp

[David Blaikie]

On Wed, Jan 16, 2013 at 10:46 AM, Kevin Enderby <enderby at apple.com> wrote:
>
> On Jan 16, 2013, at 10:38 AM, David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Wed, Jan 16, 2013 at 9:55 AM, Kevin Enderby <enderby at apple.com> wrote:
>>> Hi Eric,
>>>
>>> This is just for testing (without the clang change).  I didn't want to add a
>>> it as a command line argument to llvm-mc as that would then have the
>>> producer string as it would also affect the AT_Apple flags.
>>
>> Curious: what do you mean by this? What are the AT_Apple flags?
>> (sorry, I'm not familiar with this area)
>
> The AT_APPLE_flags are set to the command line arguments when the
> environment variable RC_DEBUG_OPTIONS is set.  We use it for checking
> for compiler command line arguments in binaries.

Hmm - but it'd only be in there if someone passed it in, right. What's
the problem with that? (again, I don't mean to imply there isn't a
problem - I'm by no means well versed in this area)

It still wouldn't effect Clang, would it? Since Clang is using this at
the API level, not actually invoking the command with command line
arguments, yes?

>> As for the testing issue, could this be written as a unit test instead
>> of having a test hook like this?
>
> I'm not sure what you are asking for.  I did create a test case for this that
> uses just llvm-mc.  Are you like Eric and don't want it tested with an environment
> variable?

It sounded like Eric was OK with this strategy as a placeholder until
you have the Clang functionality/tests & wanted you to remove it after
that (perhaps I misread Eric, though). I'd rather not lose test
coverage in LLVM, so if Eric's not OK with the environment variable
support remaining in perpetuity (& honestly I'm not a huge fan of
using environment variables as test hooks in production code) then a
unit test may be more appropriate.

>
>> (Ideally even once Clang is testing
>> this code path, it'd be nice to still have LLVM functionality tested
>> in LLVM so LLVM developers who aren't necessarily working
>> with/building/testing Clang could still know that they haven't broken
>> this functionality - but I realize sometimes that benefit isn't worth
>> the overhead of writing such tests for relatively trivial features,
>> but it's worth checking* if we've reached a tipping point where a few
>> trivial features could all benefit from the addition of unit tests,
>> for example)
>>
>> * I say this in the absence of any specific knowledge - perhaps this
>> is the only such case & there's no aggregate benefit, etc, at the
>> moment.
>>
>>>
>>> If you like I can remove this code and the test when the clang side of the
>>> change is finished.
>>>
>>> Kev
>>>
>>> On Jan 16, 2013, at 9:52 AM, Eric Christopher <echristo at gmail.com> wrote:
>>>
>>>
>>>
>>>> +static std::string DwarfDebugProducer;
>>>> +static void setDwarfDebugProducer(void) {
>>>> +  if(!getenv("DEBUG_PRODUCER"))
>>>> +    return;
>>>> +  DwarfDebugProducer += getenv("DEBUG_PRODUCER");
>>>> +}
>>>> +
>>>
>>>
>>> Any particular reason or compatibility with this? I'd prefer not to have it
>>> otherwise. If we do need it can you make it work for the compiled case as
>>> well?
>>>
>>> Thanks!
>>>
>>> -eric
>>>
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>
>