This is an archive of the discontinued LLVM Phabricator instance.

[llvm] Implement support for -defsym assembler option
AbandonedPublic

Authored by mgrang on Nov 1 2016, 2:05 PM.

Diff Detail

Event Timeline

mgrang updated this revision to Diff 76628.Nov 1 2016, 2:05 PM
mgrang retitled this revision from to [llvm] Implement support for --defsym assembler option.
mgrang updated this object.
mgrang updated this object.Nov 1 2016, 3:13 PM
colinl edited edge metadata.Nov 1 2016, 3:15 PM

LGTM after changing to a forward declaration.

include/llvm/MC/MCContext.h
20

MCAsmParser should be forward declared instead of including the header.

mgrang added inline comments.Nov 1 2016, 4:55 PM
include/llvm/MC/MCContext.h
20

Forward declaration does not work in this case since the definition of MCAsmParser is required.

mgrang updated this revision to Diff 76656.Nov 1 2016, 4:58 PM
mgrang edited edge metadata.

Changed the type of "Value" in function setSymbolValue to int64_t.

mgrang updated this revision to Diff 76754.Nov 2 2016, 11:38 AM

Replaced inclusion of MCAsmParser.h in MCContext.h with forward declaration of "class MCAsmParser".
Included MCAsmParser.h in MCContext.h instead.

mgrang updated this revision to Diff 76782.EditedNov 2 2016, 2:02 PM

Minor fix:

  • static int fillCommandLineSymbols(MCAsmParser &Parser){ + static int fillCommandLineSymbols(MCAsmParser &Parser) {
mgrang marked an inline comment as done.Nov 7 2016, 9:34 AM

Ping!

mgrang updated this revision to Diff 77090.Nov 7 2016, 1:28 PM

Changed setSymbolValue to use Streamer directly instead of getting it from Parser.

Reviewers,
Can we please get this and the related (https://reviews.llvm.org/D26213) patches reviewed.
We need these patches to support an internal customer.

Thanks,
Mandeep

colinl added inline comments.Nov 10 2016, 1:17 PM
lib/MC/MCContext.cpp
263

It's probably more idomatic to put this method on MCStreamer and pass in an MCContext reference.

mgrang added inline comments.Nov 10 2016, 1:34 PM
lib/MC/MCContext.cpp
263

I guess the setSymbolValue is better fit in MCContext since there are similar functions there
and all those functions are on MCContext.

rengolin requested changes to this revision.Nov 14 2016, 4:12 AM
rengolin edited edge metadata.

All the functionality tests need to be here. There are none.

In the Clang change, you need to make sure that the information is passed down, via target-features, -Wa and a dump of the values.

Here, you need to create tests that make use of those options (remember, clang is not always available when you build LLVM, so the test can't rely on that).

This revision now requires changes to proceed.Nov 14 2016, 4:12 AM

@rengolin The -defsym option is already handled by llvm-mc (see http://llvm.org/viewvc/llvm-project?view=revision&revision=239240). This patch already added llvm-mc tests for defsym.

My patch makes the assembler (cc1as) recognize this option. Hence I put the assembly checks in the clang tests.

mgrang retitled this revision from [llvm] Implement support for --defsym assembler option to [llvm] Implement support for -defsym assembler option.Nov 30 2016, 1:37 PM
mgrang edited edge metadata.
mgrang set the repository for this revision to rL LLVM.
mgrang added a subscriber: llvm-commits.
rengolin accepted this revision.Dec 1 2016, 7:21 AM
rengolin edited edge metadata.

Sorry, fell out of my radar. I really can't see how the tests would be moved in here.

LGTM.

This revision is now accepted and ready to land.Dec 1 2016, 7:21 AM
This revision was automatically updated to reflect the committed changes.

@rengolin @compnerd Thanks for the reviews!

grosbach reopened this revision.Dec 1 2016, 11:09 AM
grosbach added a subscriber: grosbach.
grosbach added inline comments.
llvm/trunk/lib/MC/MCContext.cpp
263 ↗(On Diff #79948)

The error checking shouldn't be in the setter function, but rather in the caller.

266 ↗(On Diff #79948)

The MC layer should not be writing directly to errs(), but rather going through the diagnostics handlers.

This revision is now accepted and ready to land.Dec 1 2016, 11:09 AM
grosbach requested changes to this revision.Dec 1 2016, 11:11 AM
grosbach added a reviewer: grosbach.

Please revert and fix.

This revision now requires changes to proceed.Dec 1 2016, 11:11 AM
mgrang added inline comments.Dec 1 2016, 5:04 PM
llvm/trunk/lib/MC/MCContext.cpp
263 ↗(On Diff #79948)

@grosbach Thanks for your comments.

This function is called from llvm-mc as well as from cc1as_main in clang. Is it OK if I create an error checker function here and call it from both the call sites? Otherwise we would end up duplicating the error checking logic in both call sites.

266 ↗(On Diff #79948)

@grosbach I will change this to go through diagnostics handlers.

However, I just moved this function from llvm-mc.cpp as is (almost). llvm-mc was already writing to errs(). Also I see places in MC where we directly write to errs(), like in MC/SubtargetFeature.cpp.

echristo added inline comments.Dec 1 2016, 5:19 PM
llvm/trunk/lib/MC/MCContext.cpp
263 ↗(On Diff #79948)

The applications often have their own error handling. If you want to put something in Support that can be easily handled it's fine, but otherwise it's best if you just duplicate it. Happy to look at code that does either.

266 ↗(On Diff #79948)

llvm-mc is an application and not a library. MC itself should not be writing to errs().

The uses in SubtargetFeature should be fixed, not promulgated further.

grosbach added inline comments.Dec 1 2016, 5:25 PM
llvm/trunk/lib/MC/MCContext.cpp
263 ↗(On Diff #79948)

That's not the function of MCContext. The diagnostic checking really should be in the callers even if it's duplicated. Consider, for example, that llvm-mc is a development testing tool for LLVM developers and clang is an end-user tool. It is entirely possible that we would want different checking and behaviours for those use cases.

266 ↗(On Diff #79948)

llvm-mc writing to errs() is fine. Stuff in lib/MC is not. The former is a development and testing tool. The latter is production library code.

The majority of the references to errs() in SubtargetFeature.cpp are also bugs and should be fixed.

mgrang added a comment.Dec 1 2016, 5:34 PM

@grosbach @echristo Thanks for the explanations. I will push a fix.

t.p.northover added inline comments.Dec 1 2016, 8:46 PM
llvm/trunk/lib/MC/MCContext.cpp
263 ↗(On Diff #79948)

The diagnostic checking really should be in the callers even if it's duplicated

Really, the MC layer shouldn't even be in the business of parsing a user-facing Clang option like this (even if it's guaranteed error-free). Users should provide the symbol and the value separately.

Patch no longer needed/valid.

mgrang abandoned this revision.Sep 7 2017, 10:59 AM

Patch no longer needed/valid.