Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix alignment in unpacked structs containing unions. (Closes: #47) #48

Closed

Conversation

lechner
Copy link

@lechner lechner commented Jun 7, 2024

The 2.0.1 release suffered from a test failure [1] that caused some distributions to disable the test [2] but the underlying cause had yet to be identified.

Based on a preliminary review of the code, the author of this commit believes there was a surplus line in the code [3] which is deleted here. This is the proposed evidence: The line shadowed the 'alignment' variable in line immediately above it. [4] Then, the same expression was used two lines down below. [5] In combination, the line being removed here seems to have been left over from editing under time pressure. It may also have been added inadvertently.

On GNU Guix, this commit fixes the 'ref1' test for aligned struct bytestructures containing union members, which was referenced via "run-tests.body.scm:194: FAIL ref1" in Ludo's original report. [6] The test output can be found at the bottom of this message.

This commit closes Github issue #47. [7]

[1] https://issues.guix.gnu.org/54165#1
[2] alpinelinux/aports@2d86046
[3]

(alignment (pack-alignment pack alignment))

[4]
(let* ((alignment (apply max (map field-spec-alignment field-specs)))

[5]
(position (align position size (pack-alignment pack alignment))))

[6]
(test-eqv "ref1" 321 (bytestructure-ref bs 'a))

[7] #47


Group begin: aligned
Test begin:
  test-name: "ref1"
  source-file: "run-tests.body.scm"
  source-line: 194
  source-form: (test-eqv "ref1" 321 (bytestructure-ref bs (quote a)))
Test end:
  result-kind: pass
  actual-value: 321
  expected-value: 321
Test begin:
  test-name: "ref2"
  source-file: "run-tests.body.scm"
  source-line: 196
  source-form: (test-eqv "ref2" 456 (bytestructure-ref bs (quote b)))
Test end:
  result-kind: pass
  actual-value: 456
  expected-value: 456
Test begin:
  test-name: "set1"
  source-file: "run-tests.body.scm"
  source-line: 197
  source-form: (test-eqv "set1" 789 (begin (bytestructure-set! bs (quote a) 789) (bytestructure-ref bs (quote a))))
Test end:
  result-kind: pass
  actual-value: 789
  expected-value: 789
Test begin:
  test-name: "set2"
  source-file: "run-tests.body.scm"
  source-line: 199
  source-form: (test-eqv "set2" 987 (begin (bytestructure-set! bs (quote b) 987) (bytestructure-ref bs (quote b))))
Test end:
  result-kind: pass
  actual-value: 987
  expected-value: 987
Group end: aligned

…B#47)

The 2.0.1 release suffered from a test failure [1] that caused some
distributions to disable the test [2] but the underlying cause had yet
to be identified.

Based on a preliminary review of the code, the author of this commit
believes there was a surplus line in the code [3] which is deleted
here.  This is the proposed evidence: The line shadowed the
'alignment' variable in line immediately above it. [4] Then, the same
expression was used two lines down below. [5] In combination, the line
being removed here seems to have been left over from editing under
time pressure. It may also have been added inadvertently.

On GNU Guix, this commit fixes the 'ref1' test for aligned struct
bytestructures containing union members, which was referenced via
"run-tests.body.scm:194: FAIL ref1" in Ludo's original report. [6] The
test output can be found at the bottom of this message.

This commit closes Github issue TaylanUB#47. [7]

[1] https://issues.guix.gnu.org/54165#1
[2] alpinelinux/aports@2d86046
[3] https://github.com/TaylanUB/scheme-bytestructures/blob/a6d5d25b26c0c5ef1f5fa38e2472fbf6a32cdf89/bytestructures/body/struct.scm#L110
[4] https://github.com/TaylanUB/scheme-bytestructures/blob/a6d5d25b26c0c5ef1f5fa38e2472fbf6a32cdf89/bytestructures/body/struct.scm#L109
[5] https://github.com/TaylanUB/scheme-bytestructures/blob/a6d5d25b26c0c5ef1f5fa38e2472fbf6a32cdf89/bytestructures/body/struct.scm#L112
[6] https://github.com/TaylanUB/scheme-bytestructures/blob/a6d5d25b26c0c5ef1f5fa38e2472fbf6a32cdf89/run-tests.body.scm#L194
[7] TaylanUB#47

* * *

Group begin: aligned
Test begin:
  test-name: "ref1"
  source-file: "run-tests.body.scm"
  source-line: 194
  source-form: (test-eqv "ref1" 321 (bytestructure-ref bs (quote a)))
Test end:
  result-kind: pass
  actual-value: 321
  expected-value: 321
Test begin:
  test-name: "ref2"
  source-file: "run-tests.body.scm"
  source-line: 196
  source-form: (test-eqv "ref2" 456 (bytestructure-ref bs (quote b)))
Test end:
  result-kind: pass
  actual-value: 456
  expected-value: 456
Test begin:
  test-name: "set1"
  source-file: "run-tests.body.scm"
  source-line: 197
  source-form: (test-eqv "set1" 789 (begin (bytestructure-set! bs (quote a) 789) (bytestructure-ref bs (quote a))))
Test end:
  result-kind: pass
  actual-value: 789
  expected-value: 789
Test begin:
  test-name: "set2"
  source-file: "run-tests.body.scm"
  source-line: 199
  source-form: (test-eqv "set2" 987 (begin (bytestructure-set! bs (quote b) 987) (bytestructure-ref bs (quote b))))
Test end:
  result-kind: pass
  actual-value: 987
  expected-value: 987
Group end: aligned
@TaylanUB
Copy link
Owner

TaylanUB commented Jun 7, 2024

Hi Felix, thanks a lot for looking into it. The shadowing of the variable was intentional (since the "untreated" alignment value isn't needed after that point) but then it seems I forgot that I already "treated" the alignment value by shadowing the original variable, and called pack-alignment on it again.

Simply deleting the shadowing line works, but keeping that line and changing the expression (pack-alignment pack alignment) two lines below to just alignment would be marginally cleaner I think.

I'll implement the fix and make a release ASAP. Someone else will have to update the Guix package, since I'm out of the loop on the commit workflow, and also have no motivation to contribute to Guix right now due to cultural problems.

@TaylanUB TaylanUB closed this Jun 7, 2024
@TaylanUB
Copy link
Owner

TaylanUB commented Jun 7, 2024

Actually, looking a bit more at the code, I don't see how this patch could possibly change the behavior of the program. Setting alignment to (pack-alignment pack alignment) should be idempotent when pack doesn't change, so the extra pass shouldn't make a difference.

If I'm not mistaken, the test failure was only on i386 architectures. Sadly, I don't have such a system available. Have you tested it on such a system to see whether the change actually causes the tests to pass successfully? On an amd64 system, the tests all pass anyway.

@TaylanUB TaylanUB reopened this Jun 7, 2024
@lechner
Copy link
Author

lechner commented Jun 7, 2024

@TaylanUB Thanks for the quick response, and sorry about my confusion about the i386 failure. (In Guix, there may be an easy way to emulate it.) I am a very happy user of nyacc, which depends on your code.

I'll now close this pull request.

As for your experience with GNU Guix, I am sorry to read about it. I didn't look at the whole exchange, but I believe that the tone of some responses you received was inappropriate, especially the word "misguided." At least in the United States, sex is and remains a protected legal class under various Federal civil rights laws as well as under the Unruh Act in California, where I live.

Either way, I believe that the error you made, if you don't mind my being direct, was to get involved in an argument. We live in a society with certain trends. The pendulum swings one way one day, and another way later. A community as large as Guix also consists of people of all kinds and from many walks of life. The key is, I think, to focus on writing great software. You should be proud that your work product is one such piece of great software!

Will you please amend the bug with a reference to the relevant messages on Github? Please do not forget to add a patch updating the version to 2.0.2. I'd be happy to do so but would love to have you back at Guix. Thanks!

Kind regards
Felix

@lechner lechner closed this Jun 7, 2024
@TaylanUB
Copy link
Owner

TaylanUB commented Jun 8, 2024

I released 2.0.2 a couple minutes earlier, which shouldn't fail on i386 anymore. I wrote a few more comments here: #47

Thank you for the kind words. Sadly, I see the problem as being severe enough to warrant my decision to stay away from Guix for now.

My goal in starting that thread on the Guix ML was to push Guix towards a position of neutrality on a controversial topic. I've repeatedly clarified this, saying that I wanted Guix to take a neutral position, and not one in favor of the views that I see as being the right ones.

I've also done my best to be empathetic, and don't believe I've said anything that could be construed as hurtful. (Except insofar some people are hurt by any statement contradicting their ideologies. Similar to how, for instance, a deeply religious person may be hurt by the mere statement of one's non-belief.)

That didn't save me from severe vilification, and ultimately, the decision of the maintainers to remove me from the list of trusted members, after I had written that I was done responding and had no intention of talking about the topic any more.

This signals to me that 1. Guix is an exclusive community which only tolerates members who adopt certain political ideologies (that I see as being misogynist, homophobic, and anti-scientific), and 2. the maintainers wanted to publicly punish me just to appease an angry mob.

Currently, I don't see myself returning to the project, unless the maintainers make a statement explicitly declaring the project's neutrality on political topics that aren't related to Free Software.

All the best,

Taylan

@lechner
Copy link
Author

lechner commented Jun 8, 2024

Please forgive me, but you sound hurt, too. You shouldn't be. You just advocated against it. Be strong and come back.

Other projects are much worse. As you noted, it's a larger issue of prevailing opinion. Also, I'd like to know: Which operating system can compete with GNU Guix, please?

@TaylanUB
Copy link
Owner

TaylanUB commented Jun 8, 2024

Well yes, I was/am quite hurt. I've been hounded out of a number of places by such zealots by now and don't really have much patience left for them. There are people who lost their jobs, livelihood, and close friends over this issue, and I've actually gotten away lightly in comparison, because I'm quite unsocial to begin with... Still, seeing that even the Guix community, which until then I considered to be made up of very kind and intelligent people, has fallen for this cult-like social phenomenon ("wokeness" for lack of a better term) was quite a shock.

Sometimes I get the feeling that the entire Free Software movement is currently split into two main camps: the "woke" zealots, and heavily right-leaning "free speech absolutist" types who can't stand still for two seconds without hurling around racial slurs. (The latter group is probably much smaller.) Where are all the sane people at? If I had a conspiratorial mindset, I'd say this must all be a psyop by Big Tech. (joking)

I generally use Debian on servers (like for hosting FeministWiki and bg3.wiki), as a sort of "can't go wrong with" option. At times I've used Ubuntu LTS which is quite similar. Using these also means I don't have to deal with providing a custom installation image when I'm renting a server, like from Hetzner.

For personal computing, I've sadly been stuck with MS Windows for a while. I've been meaning to switch again though, since I quit my last job that sort of required it. I may end up using Guix again, purely as a user, or start playing with something else to expand my horizons; I don't know yet. I've also made the mistake of buying an Nvidia graphics card just so I could play Baldur's Gate 3, so that may limit my options.

Anyway, "thanks for coming to my TED talk" I guess. :-)

@lechner
Copy link
Author

lechner commented Jun 8, 2024

One day when you are done feeling sorry for yourself I may tell you what happened to me. Or maybe some of the dozens of people who did it will tell (although I doubt it). Either way, the center you say you support is empty without you.

@TaylanUB
Copy link
Owner

TaylanUB commented Jun 8, 2024

I'd certainly like to hear your story. If you want, we can move to a different platform as well. I can be reached as @taylan@pl.tkammer.de on the Fediverse, or taylan.kammer@gmail.com. I'll probably be a bit busy in the coming days though so I may be slow to respond.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants