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

Build broken by python error in aarch64/codec.py: No attribute 'split' on Tuple #6580

Closed
derekbruening opened this issue Jan 24, 2024 · 9 comments · Fixed by #6592
Closed

Comments

@derekbruening
Copy link
Contributor

Our internal build, once past the breakage from #6575, is still broken when trying to pull in the latest Github sources, on this error:

File "core/ir/aarch64/codec.py", line 73, in opnd_stem: No attribute 'split' on Tuple[List[str], List[str]] [attribute-error]
  In Union[Any, Tuple[List[str], List[str]], str]
Called from (traceback):
  line 782, in current file
  line 758, in main
  line 393, in generate_encoder

For more details, see [http://go/pytype-errors#attribute-error](https://www.google.com/url?q=http://go/pytype-errors%23attribute-error&sa=D)
@derekbruening
Copy link
Contributor Author

I've locked the master branch for now to prevent further issues and reduce the amount of churn until the build issues are resolved

@derekbruening derekbruening changed the title Build broken by python error in aarch64/codec.py: No attribute 'split' Build broken by python error in aarch64/codec.py: No attribute 'split' on Tuple Jan 24, 2024
@derekbruening
Copy link
Contributor Author

The culprit is PR #6535 960d1d7

@joshua-warburton
Copy link
Collaborator

I've had a quick look into this. I would expect this to fail if presented with a tuple, but it seems that the line it's failing on expects a string and would fail if it got a tuple even before the change. I've followed the code through and don't see anything in the patch itself that would cause this to fail as by this point we've re-written the opndsets with the function opndset_naming to remove the lists.

I can try to reproduce this if you have the python version you're using and which ref you're trying to build

@joshua-warburton
Copy link
Collaborator

joshua-warburton commented Jan 24, 2024

Detail:
The build is failing because we attempt to run .split() on a tuple, pattern.opndset via the opnd_stem() function. However, if we remove that function we're attempting to encode a tuple as a string, which will fail still with:

>>> t = "%s"
>>> a = ("a", "b", "c")
>>> t %a
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: not all arguments converted during string formatting

Tracing back, pattern.opndset is initially populated by either the highlighted arguments below

patterns.append(Pattern(pattern, opcode_bits, opnd_bits, high_soft_bits, opcode, ***(dsts, srcs)***, enum, feat))
patterns.append(Pattern(pattern, opcode_bits, opnd_bits, high_soft_bits, opcode, ***opndset***, enum, feat))

This is later flattened in the function opndset_naming on line 742. It takes both the string and non-string variants and rewrites these as something like "gen_04205000_001f07ff" and guarantees it will be a string from there forward.

Clearly, something is getting past this check but I'm not sure how and the patch mentioned above hasn't changed any of this.

@derekbruening
Copy link
Contributor Author

This is trying to update from 0a6c057 to 3772e01. I only looked at the git blame for the split to identify the supposed culprit: did not do a bisect or look further to see if that PR was just moving stuff around. Looks like this error is actually from a static analyzer on codec.py rather than an actual runtime error: it's pytype https://google.github.io/pytype/

@joshua-warburton
Copy link
Collaborator

That would make sense Codec.py is structured in a way that's hard for static analysers to deal with. There isn't an easy way to restructure the code to remove the variable reuse that's likely causing the static analyser to dislike this.

We could selectively type annotate the functions, but I'd be hesitant about that breaking compatibility with older pythons.

I'd recommend adding a disabling comment for the checker:

    return opnd_name.split(".")[0] # pytype: disable=attribute-error

@derekbruening
Copy link
Contributor Author

We may try to disable this pytype check temporarily to unblock internal and github progress so getting codec.py to not trigger pytype errors can be addressed separately.

@derekbruening
Copy link
Contributor Author

I'd recommend adding a disabling comment for the checker:

    return opnd_name.split(".")[0] # pytype: disable=attribute-error

Thanks, that does seem to work.

derekbruening added a commit that referenced this issue Jan 26, 2024
Disable a pytype static analysis check on opnd_stem() in
aarch64/codec.py as the analysis produces a false positive.

Tested on an internal build where pytype is enabled.

Fixes #6580
joshua-warburton added a commit that referenced this issue Jan 26, 2024
This patch refactors how the script gets hold of the name of generated
functions. Previously there was a pass in opndset_naming() that
overwrote the value of the opndset attribute. This was causing issues
with type checking because this changed the type of an opndset from a
tuple or string to always a string which was flagged as an error when
.split() was used on it.

As part of this change the fall-through code became completely
dead code and has been removed. It was obsoleted originally by the
addition of + masks for the operands.

It was also revealed by this change that several encodings in the
codecs were no longer used and represented less correct versions of
other encodings. They have been removed.

issue: #6580

Change-Id: I36f410e72cb0c3ac93cf6b3dff8e4199b85d688a
@joshua-warburton
Copy link
Collaborator

I've spent a bit of time refactoring the code to remove the variable reuse and clean up some of the dead code:
#6595

derekbruening added a commit that referenced this issue Jan 26, 2024
Disable a pytype static analysis check on opnd_stem() in
aarch64/codec.py as the analysis produces a false positive.

Tested on an internal build where pytype is enabled.

Fixes #6580
joshua-warburton added a commit that referenced this issue Jan 29, 2024
…ow (#6595)

This patch refactors how the script gets hold of the name of generated
functions. Previously there was a pass in opndset_naming() that
overwrote the value of the opndset attribute. This was causing issues
with type checking because this changed the type of an opndset from a
tuple or string to always a string which was flagged as an error when
.split() was used on it.

As part of this change the fall-through code became completely dead code
and has been removed. It was obsoleted originally by the addition of +
masks for the operands.

It was also revealed by this change that several encodings in the codecs
were no longer used and represented less correct versions of other
encodings. They have been removed.

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

Successfully merging a pull request may close this issue.

2 participants