Page MenuHomePhabricator

Fix Bug 11740: Turn off -g when there are directive file debug info

Authored by zhizhouy on May 5 2016, 4:28 PM.



For the bug 11740:
bug 11740

If there is already debug info in the assembly file, and user hope to use -g option for
compiling, we think we should not directly report an error.

According to what GNU assembler did, it just reused the debug info in the assembly file,
and turned off the DEBUG_TYPE option so that there will be no new debug info emitted
by assembler. This fix is just as what GNU assembler did.

The concern is the situation that there are two .text sections in the assembly file, one with
debug info and the other one without. Currently with this fix, the assembler will no longer
generate any debug info for the second .text section. And this is what GNU assembler
exactly did for this situation. So I think this still make some sense.

Diff Detail


Event Timeline

zhizhouy updated this revision to Diff 56369.May 5 2016, 4:28 PM
zhizhouy retitled this revision from to Fix Bug 11740: Turn off -g when there are directive file debug info.
zhizhouy updated this object.
zhizhouy added a reviewer: echristo.
zhizhouy added a subscriber: llvm-commits.

What about other pieces of the debug info (when generating debug info for assembly we generate labels at least, maybe other things?) - will they be appropriately disabled by the presence of existing debug info in the assembly?

2 ↗(On Diff #56369)

What's this second run line for?

I think the turning off of the GenDwarfForAssembly at the parsing stage
will lead to emitting no new debug info by the assembler at emitting stage.
So this still works for other debug info.

2 ↗(On Diff #56369)

Just to run twice with different command line options for stability. The same as what
is in the test/MC/AsmParser/directive_file.s

zhizhouy added inline comments.May 16 2016, 3:02 PM
2 ↗(On Diff #56369)

In fact I am not quite familiar with the other test case (directive_file.s),
it seems it has these two lines to face two different situations.

So I just reused it since this test case should also work as the same as
the former one.

dblaikie added inline comments.May 20 2016, 12:10 PM
2 ↗(On Diff #56369)

I'd rather not cargo cult the test in - if we don't know why it's there, could you leave it out?

3 ↗(On Diff #56369)

Might be worth describing what the behavior/issue is that's being tested here.

zhizhouy updated this revision to Diff 57978.May 20 2016, 1:21 PM

Updated the test file, removed the useless test command,
and added some description about the test case.

dblaikie accepted this revision.May 23 2016, 9:18 AM
dblaikie added a reviewer: dblaikie.

Sounds good - thanks

This revision is now accepted and ready to land.May 23 2016, 9:18 AM

Hi David,

Since I currently don't have access to submit, would you please submit
this patch for me?


This revision was automatically updated to reflect the committed changes.