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.

Sign up for GitHub

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

Jump to bottom

getelementptr inbounds clarifications #65478

Merged
merged 1 commit into from
Sep 7, 2023
Merged

getelementptr inbounds clarifications #65478

merged 1 commit into from
Sep 7, 2023

Conversation

RalfJung
Copy link
Contributor

@RalfJung RalfJung commented Sep 6, 2023 •
edited

  • For a long time I assumed that inbounds means "in-bounds of a live allocation". @nikic told me that is not correct. I think this definitely needs clarification in the docs.
  • The point about successively adding the offsets to the current address confused be because it talked about the successive addition of "an" offset -- which one? My interpretation was, the total accumulated offset computed in the previous step. But @nikic told me that's not correct, adding each offset individually has to stay in-bounds for each step. I hope by saying "each offset" this becomes more clear; I then also change the previous bullet to use the same terminology.

@RalfJung RalfJung requested a review from a team as a code owner September 6, 2023 13:28
nunoplopes
nunoplopes reviewed Sep 7, 2023
View reviewed changes
llvm/docs/LangRef.rst Outdated Show resolved Hide resolved
@nunoplopes
Copy link
Member

LGTM!
Agreed objects may not be live anymore. This allows free movement of geps.

nunoplopes
nunoplopes reviewed Sep 7, 2023
View reviewed changes
llvm/docs/LangRef.rst Outdated
@@ -10955,15 +10955,17 @@ If the ``inbounds`` keyword is present, the result value of a
:ref:`poison value <poisonvalues>` if one of the following rules is violated:

* The base pointer has an *in bounds* address of an allocated object, which
means that it points into an allocated object, or to its end.
means that it points into an allocated object, or to its end. Note that the
object does not have to be live any more; being in-bounds of a deallocated
Copy link
Member

Choose a reason for hiding this comment

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

should be anymore

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, fixed.

@nunoplopes nunoplopes merged commit 1c567a5 into llvm:main Sep 7, 2023
1 of 2 checks passed
@RalfJung
Copy link
Contributor Author

RalfJung commented Sep 8, 2023

I'm now receiving a bunch of scary-sounding emails:
"Buildbot failure in LLVM Buildbot on flang-aarch64-out-of-tree"
"Buildbot failure in LLVM Buildbot on sanitizer-x86_64-linux-qemu"
"Buildbot failure in LLVM Buildbot on clang-s390x-linux"
Is it safe for me to ignore them...?

@RalfJung RalfJung deleted the getelementptr-inbounds branch September 8, 2023 06:14
@nunoplopes
Copy link
Member

Yes, just ignore those. First, buildbots group multiple commits. You'll see that on the "blame list".
Second, some of the sanitizer bots fail randomly.
A change to LangRef can't fail any buildbot.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nunoplopes nunoplopes nunoplopes approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@RalfJung @nunoplopes