This is an archive of the discontinued LLVM Phabricator instance.

MCObjectStreamer: assign MCSymbols in the dummy fragment to offset 0.
ClosedPublic

Authored by jyknight on Nov 10 2019, 1:33 PM.

Details

Summary

In MCObjectStreamer, when there is no current fragment, initially
symbols are created in a "pending" state and assigned to a dummy
empty fragment.

Previously, they were not being assigned an offset, and thus
evaluateAbsolute would fail if trying to evaluate an expression 'a -
b', where both 'a' and 'b' were in this pending state.

Fixes: https://llvm.org/PR41825

Diff Detail

Event Timeline

jyknight created this revision.Nov 10 2019, 1:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 10 2019, 1:33 PM

This looks reasonable to me. The one concern I had was that Checking over the case where getRelaxAll() is true, it looks like each instruction is created in its own fragment, then merged with the previous, triggering a call to flushPendingLabels which will set the offset correctly before the temporary offset of 0 could be misused. Might be worth a test case for the combination of .if and --mc-relax-all.

llvm/lib/MC/MCObjectStreamer.cpp
270–271

For consistency is it worth setting the offset to 0 here as well? From what I can tell this is only used for outputting Arm/Thumb mapping symbols and these really shouldn't be used for any kind of symbolic lookup, but it may be possible other uses for this function will be found in the future.

jyknight updated this revision to Diff 228738.Nov 11 2019, 10:41 AM
jyknight marked an inline comment as done.

Update for comment.

llvm/lib/MC/MCObjectStreamer.cpp
270–271

Agreed.

This function's semantics seem kinda broken, since sometimes it sets offset and sometimes doesn't. (shouldn't matter for the only current usage). So, refactored a bit, and done.

llvm/test/MC/AsmParser/assembler-expressions.s
36

This is testing that an error is emitted? Don't we want to test that an error is *not* emitted?

Thanks for this patch!

llvm/test/MC/AsmParser/assembler-expressions.s
36

I got a feeling this test case has something to do with my patch at https://reviews.llvm.org/D69411 :). I accidentally removed a nop instruction in the test case used to reproduce the issue in the said patch. Will update my test case. Thanks @jyknight for catching it.

jyknight marked 3 inline comments as done.Nov 11 2019, 11:47 AM
jyknight added inline comments.
llvm/test/MC/AsmParser/assembler-expressions.s
36

No, that error is emitted when attempting to write textual assembly output (the first RUN line in the test), from parsing the textual input. In that case, we cannot evaluate differences between symbols, since we don't track any instruction sizes or anything, but that is OK -- it's irrelevant in real-world use-cases.

The other two RUN lines, where we emit an object file, succeed.

jyknight marked an inline comment as done.Nov 11 2019, 11:51 AM
MaskRay accepted this revision.EditedNov 11 2019, 4:32 PM

In MCObjectStreamer, when there is no current fragment, initially symbols are created in a "pending" state and assigned to a dummy empty fragment.

Maybe clarify that "pending" means SymContentsUnset.

The following snippet (contained in assembler-expressions.s) fails before this change.

.text
text1:
# In MCExpr.cpp, AttemptToFoldSymbolOffsetDifference cannot fold because MCValue(.).SymA is SymContentsUnset (and also MCValue(text1).SymB)
.if . - text1 == 0
	nop
.endif

The second .if does not emit an error.

.text
text1:
.if . - text1 == 0
	nop
.endif

text2:
        nop
# This does not error, because MCObjectStreamer.cpp EmitLabel changes the MCValue(.).SymA to SymContentsOffset.
.if . - text2 == 1
	nop
.else
	ret
.endif

Probably wait for a sign-off from other reviewers.

This revision is now accepted and ready to land.Nov 11 2019, 4:32 PM

I'm happy with this too. I checked the ARM Mapping Symbol code, and it only uses the EmitLabelAtPos in one specific circumstance, a pending data mapping symbol which always has a MCDataFragment associated with it.

nickdesaulniers accepted this revision.Nov 12 2019, 9:24 AM

Thanks, I tested the first two cases in https://bugs.llvm.org/show_bug.cgi?id=41825. I also did not see any more patterns of EmitLabel() followed by setOffset() in the tree. @peter.smith provides an excellent summary in https://reviews.llvm.org/D69411#1742030.

This revision was automatically updated to reflect the committed changes.