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 ranges autotest take 2 #505

Merged
merged 6 commits into from
Oct 20, 2023
Merged

Conversation

sevaa
Copy link
Contributor

@sevaa sevaa commented Sep 26, 2023

The readelf in this one was built from the latest binutils' master.

The purpose of this was to remove the two file exception from the readelf..ranges test. In order to do that, I had to bring the rnglists section dump in line with what's in the latest GNU readelf master. The catch is that the rnglists section dump in GNU readelf was revised by me in https://sourceware.org/bugzilla/show_bug.cgi?id=30792 .

I'm not sure anymore what are we testing here.

It speaks to the viability of readelf as a reference implementation that the GNU binutils maintainers didn't get to it before I did.

The calling convention enum is just something I've noticed along the way and fixed.

@sevaa sevaa marked this pull request as ready for review September 26, 2023 15:26
@sevaa
Copy link
Contributor Author

sevaa commented Sep 26, 2023

Interesting datapoint: the binutils maintainer mentioned they test against eu-readelf. I don't know how much of a drop-in replacement for readelf proper would that be. Elfutils doesn't have a Cygwin build, unfortunately.

Copy link
Owner

@eliben eliben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you squash it all into a single commit that clearly states the version of readelf used for this?

@sevaa
Copy link
Contributor Author

sevaa commented Oct 16, 2023

It's a readelf built from the latest head at the time of the PR, which is no longer current anyway, 'cause I did some more work on binutils since. If you'd rather hold on until a numbered release, that's fine.

@eliben
Copy link
Owner

eliben commented Oct 16, 2023

It's a readelf built from the latest head at the time of the PR, which is no longer current anyway, 'cause I did some more work on binutils since. If you'd rather hold on until a numbered release, that's fine.

It's fine to cut the binary before a numbered release, but it should at least specify the git hash from which it's taken; I believe we've done this before.

The reason is being able to reproduce the binary from source in case this is needed (on another platform, say)

@sevaa
Copy link
Contributor Author

sevaa commented Oct 16, 2023

The hash is 84102ebc29a1ea531e7fe78bd841bfb2fe501dc2, now I have to figure out how to squish them...

@sevaa
Copy link
Contributor Author

sevaa commented Oct 17, 2023

On a side note, I have a branch of pyelftools that autotests with the latest, latest readelf, too. I think I'll hold on to those changes until the binutils team drops a numbered release. It's not all descriptions, some API changes were needed, too.

@eliben
Copy link
Owner

eliben commented Oct 17, 2023

The hash is 84102ebc29a1ea531e7fe78bd841bfb2fe501dc2, now I have to figure out how to squish them...

This was a while ago, but should probably still apply: https://eli.thegreenplace.net/2014/02/19/squashing-github-pull-requests-into-a-single-commit

@sevaa
Copy link
Contributor Author

sevaa commented Oct 17, 2023

I thought I've squashed them, but it still shows up as several commits. That's not the desired result, is it?

@eliben eliben merged commit 8c2f14e into eliben:master Oct 20, 2023
3 checks passed
@sevaa sevaa deleted the fix_rangesautotestv2 branch October 22, 2023 16:00
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.

2 participants