Issue 129540043: code review 129540043: liblink: introduce way to avoid pc-relative addressing (Closed)
Descriptionliblink: introduce way to avoid pc-relative addressing
For Solaris. Sigh.
Patch Set 1 #Patch Set 2 : diff -r c506cded50bebc3f122913bd176e3b65e43c4f37 https://code.google.com/p/go/ #Patch Set 3 : diff -r c506cded50bebc3f122913bd176e3b65e43c4f37 https://code.google.com/p/go/ #
Total comments: 2
Patch Set 4 : diff -r c29860623fb3ba526147d39e85617c7750913aa3 https://code.google.com/p/go/ #Patch Set 5 : diff -r 6199473fbef3d74e3a8ea8b537c78437888f7c3c https://code.google.com/p/go/ #MessagesTotal messages: 10
Hello aram (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://code.google.com/p/go/
Sign in to reply to this message.
https://codereview.appspot.com/129540043/diff/40001/src/liblink/asm6.c File src/liblink/asm6.c (right): https://codereview.appspot.com/129540043/diff/40001/src/liblink/asm6.c#newcod... src/liblink/asm6.c:1547: return strcmp(s->name, "runtime.main") == 0 || strcmp(s->name, "runtime·gomaxprocs") == 0; It doesn't seem like a great idea to check this in like this. This function should return true for a symbol that is defined in some other object. In other words, I think, if the symbol type is SDYNIMPORT.
Sign in to reply to this message.
https://codereview.appspot.com/129540043/diff/40001/src/liblink/asm6.c File src/liblink/asm6.c (right): https://codereview.appspot.com/129540043/diff/40001/src/liblink/asm6.c#newcod... src/liblink/asm6.c:1547: return strcmp(s->name, "runtime.main") == 0 || strcmp(s->name, "runtime·gomaxprocs") == 0; On 2014/08/25 01:44:08, iant wrote: > It doesn't seem like a great idea to check this in like this. You're right. When I wrote that I was thinking (and had implemented) that this would trigger treatment as a 64-bit address. But that didn't work for other reasons so I had to back off to 32-bit. Will remove this part. > This function should return true for a symbol that is defined in some other > object. In other words, I think, if the symbol type is SDYNIMPORT. Yes, but we don't know that. This runs during 6c, and we don't know about SDYNIMPORT until 6l.
Sign in to reply to this message.
On 2014/08/25 02:04:13, rsc wrote: > > > This function should return true for a symbol that is defined in some other > > object. In other words, I think, if the symbol type is SDYNIMPORT. > > Yes, but we don't know that. This runs during 6c, and we don't know about > SDYNIMPORT until 6l. OK, good point. For Solaris it's worth noting that I think this case will only arise when 6c sees the reference to the symbol and also sees the #pragma cgo_import_dynamic. That is, the information will be in the same file (runtime/os_solaris.c) and is therefore available in principle. Ian
Sign in to reply to this message.
>> This function should return true for a symbol that is defined in >> some other object. In other words, I think, if the symbol type is >> SDYNIMPORT. >> > Yes, but we don't know that. This runs during 6c, and we don't know > about SDYNIMPORT until 6l. Indeed, that was my original idea which I have tried, but soon I found I couldn't do it that way. > That is, the information will be in the same file (runtime/os_solaris.c) > and is therefore available in principle. Interesting idea. Would it be hard to implement this? Another idea, 6l rewrites the access to use the GOT anyway, can we do the whole transformation in 6l? In other words, change what 6l matches now. Would that be feasible? It would be less complex, since we would do special processing in a single place (6l) rather than two place (liblink and 6l). How does this work with Windows anyway? Windows uses approximately the same mechanism.
Sign in to reply to this message.
On Mon, Aug 25, 2014 at 5:36 AM, <aram@mgk.ro> wrote: > This function should return true for a symbol that is defined in >>> some other object. In other words, I think, if the symbol type is >>> SDYNIMPORT. >>> >> > Yes, but we don't know that. This runs during 6c, and we don't know >> about SDYNIMPORT until 6l. >> > > Indeed, that was my original idea which I have tried, but soon I > found I couldn't do it that way. > > > That is, the information will be in the same file >> > (runtime/os_solaris.c) > >> and is therefore available in principle. >> > > Interesting idea. Would it be hard to implement this? > It's not too hard except that cgo seems to be working on every other system, so I still think there's something anomalous about Solaris that is the fundamental problem here. Until we understand it, I don't want to make changes that could break cgo on the other systems just to "fix" Solaris. I'm really unhappy about the damage the Solaris port is doing to our source code, and I'm trying to contain it. > Another idea, 6l rewrites the access to use the GOT anyway, can we > do the whole transformation in 6l? In other words, change what 6l > matches now. Would that be feasible? It would be less complex, since > we would do special processing in a single place (6l) rather than > two place (liblink and 6l). > > How does this work with Windows anyway? Windows uses approximately > the same mechanism. > I don't know the details of how Solaris differs from Windows. Obviously it is only approximately the same. :-) Russ
Sign in to reply to this message.
Hello aram@mgk.ro, iant@golang.org (cc: golang-codereviews@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM. Thanks Russ, this has fixed the solaris/amd64 builder. On Tue, Aug 26, 2014 at 2:15 AM, <rsc@golang.org> wrote: > Hello aram@mgk.ro, iant@golang.org (cc: > golang-codereviews@googlegroups.com), > > Please take another look. > > > https://codereview.appspot.com/129540043/ > > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout.
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=02fc3509db71 *** liblink: introduce way to avoid pc-relative addressing For Solaris. Sigh. LGTM=dave R=aram, iant, dave CC=golang-codereviews https://codereview.appspot.com/129540043
Sign in to reply to this message.
Message was sent while issue was closed.
Thanks for fixing this while I was gone. > It's not too hard except that cgo seems to be working on every other > system, so I still think there's something anomalous about Solaris > that is the fundamental problem here. I am very confused by this statement. I don't understand why you mentioned cgo. The Solaris and Windows ports doesn't use cgo in the regular sense; yes, they use code and infrastructure that was written for cgo, but the actual mechanism is quite different (and much simpler). In particular, "real" cgo-enabled code uses gcc to generate the machine code that accesses functions in shared libraries (i.e. what was broken here). This is true even with internal linking. For comparison, in "faux"-cgo used by Windows and Solaris, this code is generated by 6l. Since with "real" cgo, this type of machine code is written by gcc, not 6l, I don't understand the relevancy of the fact that cgo is working on every other platform, but I admit I might be missing the larger issue here. That being said, I am certain Solaris-specific bugs might be lurking in liblink/6l. I will try to find and fix any inconsistency and idiosyncrasy. > I don't know the details of how Solaris differs from Windows. > Obviously it is only approximately the same. :-) The input is the same, the output is obviously different, but there should be an isomorphism between the transformations. Since this doesn't seem the case, it probably means there's a bug somewhere. I will hunt it down.
Sign in to reply to this message.
|
