Saleem, in 2014 you added the .thumb_set test case. I’ve found a difference in behaviour of .thumb_set compared to .set. I was hoping to resolve this difference as it will allow me to reapply r239440.
The issue i’m seeing is that we define an alpha function, then beta function, then assign alpha to beta. But given that beta has already been defined, this would mean we are redefining it at the point of the .thumb_set:
.text
.thumb
.thumb_set alias_defined_data, bedazzle
.type alpha,%function
alpha:
nop
.type beta,%function
beta:
bkpt
.thumb_set beta, alpha
The above code currently passes. However, if I change .thumb_set to .set then I get
…/test/MC/ARM/thumb_set.s:60:13:error:redefinition of ‘beta’
.set beta, alpha
I would like to make .thumb_set throw the same error here as I believe that is how it should behave in this case.
Please let me know if you are ok with this, or some other approach. I’m unfamiliar with it so its possible that what i’ve found isn’t a bug at all but just how we want it to behave.
For what it is worth it, gas has this strange behaviour.
I agree it is strange, but why is it an issue for r239440? Will I get
a failure if I revert it?
You will yeah. beta had its ‘Offset’ set to some value but then the .set tried to make it a variable using setVariableValue.
Given that I put Offset and Value in the same union, I added an assert to ensure that we can’t change from Offset to Value.
Do you think its possible for this change from Offset to Value to happen to a symbol? If so, and if Offset and Value are still mutually exclusive then we just need to support overriding the Offset with a Value.
You will yeah. beta had its ‘Offset’ set to some value but then the .set tried to make it a variable using setVariableValue.
Given that I put Offset and Value in the same union, I added an assert to ensure that we can’t change from Offset to Value.
Do you think its possible for this change from Offset to Value to happen to a symbol? If so, and if Offset and Value are still mutually exclusive then we just need to support overriding the Offset with a Value.
I think so, that is what .thumb_set is doing, it is replacing a
regular symbol with a variable.
For what it is worth, just deleting the asserts gets all tests to pass
You will yeah. beta had its ‘Offset’ set to some value but then the .set tried to make it a variable using setVariableValue.
Given that I put Offset and Value in the same union, I added an assert to ensure that we can’t change from Offset to Value.
Do you think its possible for this change from Offset to Value to happen to a symbol? If so, and if Offset and Value are still mutually exclusive then we just need to support overriding the Offset with a Value.
I think so, that is what .thumb_set is doing, it is replacing a
regular symbol with a variable.
Ah I see.
So I think it would be ok to do this replacement prior to any calls to MCSymbol::getOffset(). It looks like we don’t call that until we are emitting the object file, i.e., we don’t call it during the AsmParser. So I think you’re right that this will be safe.
I’ll double check all the places which make use of the variable just to ensure that they always override any use of the offset.
For what it is worth, just deleting the asserts gets all tests to pass
Yeah, i was tempted to sneak it in without the asserts
Saleem, in 2014 you added the .thumb_set test case. I’ve found a
difference in behaviour of .thumb_set compared to .set. I was hoping to
resolve this difference as it will allow me to reapply r239440.
The issue i’m seeing is that we define an alpha function, then beta
function, then assign alpha to beta. But given that beta has already been
defined, this would mean we are redefining it at the point of the
.thumb_set:
.text
.thumb
.thumb_set alias_defined_data, bedazzle
.type alpha,%function
alpha:
nop
.type beta,%function
beta:
bkpt
.thumb_set beta, alpha
The above code currently passes. However, if I change .thumb_set to .set
then I get
*../test/MC/ARM/thumb_set.s:60:13: **error: **redefinition of 'beta'*
.set beta, alpha
I would like to make .thumb_set throw the same error here as I believe
that is how it should behave in this case.
Please let me know if you are ok with this, or some other approach. I’m
unfamiliar with it so its possible that what i’ve found isn’t a bug at all
but just how we want it to behave.
The behavior was taken from GAS. I don't know if there are applications
using this and dependent on the behavior. Given that this is a GNU
extension, I think its better to keep the behavior similar to GAS rather
than implement and diverge.
The behavior was taken from GAS. I don’t know if there are applications
using this and dependent on the behavior. Given that this is a GNU
extension, I think its better to keep the behavior similar to GAS rather
than implement and diverge.
I kind of agree with Saleem, here. We don’t want to deviate, even from
silly behaviour, when implementing a GNU extension.
"This performs the equivalent of a .set directive in that it creates a symbol which is an alias for another symbol (possibly not yet defined). This directive also has the added property in that it marks the aliased symbol as being a thumb function entry point, in the same way that the .thumb_func directive does.”
The first part is most important here. Its the equivalent of a .set.
I would argue that in this case it should throw exactly the same errors in exactly the same situations as .set. Any more/less and it wouldn’t be equivalent.
Sorry if that comes across as heavy handed, but i just want to be clear how i interpret the behavior here. Ideally i’d like to take whatever tests we have for .set errors/warnings and port all of them to ARM to make sure we have the same behavior.
I hate when GCC/GAS documents something and does another. When you
bring this to them all you get is "we can't change now because
legacy".
In the past, we have chosen being compatible with the docs over
bug-for-bug, so in this case, I think we better implement .thumb_set
as .set and add a big comment about this in the code, so that future
bug reports can be quickly dismissed after a short investigation.
Yeah, i hope that doesn’t happen here. but no way to know for sure if this will uncover bad code or not.
I’ve prepared a patch just to see what this would take to get the .set behavior. I first tried to refactored AsmParser::parseAssignment so that the act of parsing was separate from emitting the assignment. The we can call EmitAssignment in the AsmParser and emitThumbSet in the ARMArmParser.
Unfortunately that doesn’t work due to ARMAsmParser being a MCTargetAsmParser and so unrelated to AsmParser. Also, AsmParser itself is fully defined in a .cpp file, with no headers shared by anything else.
So, sharing any code here seems complicated. I could move the parseAssignment stuff down to the MCAsmParser but it feels unrelated to any of the other methods in that class.
Anyway here’s the patch in case you’re curious. I’ll of course need to add the comments you mentioned and tests, and await feedback about this whole issue from Saleem.
> I would argue that in this case it should throw exactly the same errors
in
> exactly the same situations as .set. Any more/less and it wouldn’t be
> equivalent.
Sigh... You are, obviously, correct.
I hate when GCC/GAS documents something and does another. When you
bring this to them all you get is "we can't change now because
legacy".
In the past, we have chosen being compatible with the docs over
bug-for-bug, so in this case, I think we better implement .thumb_set
as .set and add a big comment about this in the code, so that future
bug reports can be quickly dismissed after a short investigation.
Saleem, does that sound reasonable?
Yeah, going with the documented behavior is reasonable.
