Skip to content

Add missing include to X86MCTargetDesc.h #123320

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

Merged
merged 1 commit into from
Jan 20, 2025
Merged

Conversation

hageboeck
Copy link
Contributor

@hageboeck hageboeck commented Jan 17, 2025

In gcc-15, explicit includes of <cstdint> are required when fixed-size integers are used. In this file, this include only happened as a side effect of including SmallVector.h

Although llvm compiles fine, the root-project would benefit from explicitly including it here, so we can backport the patch.

Maybe interesting for @hahnjo and @vgvassilev

In gcc-15, explicit includes of <cstdint> are required when fixed-size integers are used.
In this file, this include only happened as a side effect of including SmallVector.h
Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write permissions for the repository. In which case you can instead tag reviewers by name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

@llvmbot
Copy link
Member

llvmbot commented Jan 17, 2025

@llvm/pr-subscribers-backend-x86

Author: Stephan Hageboeck (hageboeck)

Changes

In gcc-15, explicit includes of <cstdint> are required when fixed-size integers are used. In this file, this include only happened as a side effect of including SmallVector.h

Although llvm compiles fine, the root-project would benefit from explicitly including it here, so we can backport the patch.

Maybe interesting for @hahnjo and @vgvassilev


Full diff: https://github.com/llvm/llvm-project/pull/123320.diff

1 Files Affected:

  • (modified) llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.h (+1)
diff --git a/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.h b/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.h
index e166b68668d9b1..0e0e13e896aeaa 100644
--- a/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.h
+++ b/llvm/lib/Target/X86/MCTargetDesc/X86MCTargetDesc.h
@@ -14,6 +14,7 @@
 #define LLVM_LIB_TARGET_X86_MCTARGETDESC_X86MCTARGETDESC_H
 
 #include "llvm/ADT/SmallVector.h"
+#include <cstdint>
 #include <memory>
 #include <string>
 

@RKSimon
Copy link
Collaborator

RKSimon commented Jan 17, 2025

@hageboeck I'm not clear on the motivation for this - why isn't including via SmallVector.h acceptable for gcc15?

@hageboeck
Copy link
Contributor Author

hageboeck commented Jan 17, 2025

@hageboeck I'm not clear on the motivation for this - why isn't including via SmallVector.h acceptable for gcc15?

Hello @RKSimon,

there is a double motivation:

  1. If you follow "include what you use", you would include the standard headers to be "future proof" in case somebody would change the includes of SmallVector.h. This file would break due to its usage of fixed-size fp types if somebody removes the include from SmallVector.h. I find this enough motivation to accept this patch.
  2. The real reason I made this PR is a bit more complicated:
  • The root-project uses llvm-18, and will be able to upgrade to the next version only in several months.
  • Due to the missing includes in SmallVector.h (fixed in main) and also in X86MCTargetDesc.h (fixed here), root-project doesn't compile with gcc-15. See also https://bugs.gentoo.org/942438.
  • Now we could simply patch llvm-18 to get out of the dilemma, and never tell anybody about it. However, we try to upstream almost all our patches if possible. To work with an older version like llvm-18, we fix locally, upstream, and then backport these until we are able to upgrade. This will allow future upgrades of llvm to be rebases in which our "local" llvm patches disappear, because they are already in upstream. Let me know if I managed to explain this in an understandable manner ... 😅

In order to do the above, however, I first have to upstream the patch. Once merged, we backport it to llvm-18, and live with a "local" patch until we manage to move to the next llvm version.

@hahnjo or @vgvassilev might be able to help me explain if I didn't manage. 🙂

@hageboeck
Copy link
Contributor Author

And there is one more missing puzzle piece:

@vgvassilev
Copy link
Contributor

Generally self contained header files are a good practice. While we don't really enforce that in the LLVM development policy we implicitly required self contained header files for clang modules. It is great if gcc-15 became more strict in that regard. I think we should move forward with this trivial patch if it solves something for downstream clients.

@vgvassilev
Copy link
Contributor

And there is one more missing puzzle piece:

* The include of SmallVector.h (the one that fixes the problem via a parasitic include) only appears in [2222fdd#diff-3a97dc53c7250e03c83d10c7bd769776dc47bdcc170fddcd49f92f8b12f1e5f7R16](https://github.com/llvm/llvm-project/commit/2222fddfc0a2ff02036542511597839856289094#diff-3a97dc53c7250e03c83d10c7bd769776dc47bdcc170fddcd49f92f8b12f1e5f7R16) (llvm-20). So we can't fix llvm-18, because this patch doesn't apply.

This accidentally fixed it because we added SmallVectorImpl and had to include SmallVector.h and we got the include transitively.

@hageboeck
Copy link
Contributor Author

@abhishek-kaushik22 who should we ask to merge the PR?

@vgvassilev vgvassilev merged commit 7abf440 into llvm:main Jan 20, 2025
10 checks passed
Copy link

@hageboeck Congratulations on having your first Pull Request (PR) merged into the LLVM Project!

Your changes will be combined with recent changes from other authors, then tested by our build bots. If there is a problem with a build, you may receive a report in an email or a comment on this PR.

Please check whether problems have been caused by your change specifically, as the builds can include changes from many authors. It is not uncommon for your change to be included in a build that fails due to someone else's changes, or infrastructure issues.

How to do this, and the rest of the post-merge process, is covered in detail here.

If your change does cause a problem, it may be reverted, or you can revert it yourself. This is a normal part of LLVM development. You can fix your changes and open a new PR to merge them again.

If you don't get any reports, no action is required from you. Your changes are working as expected, well done!

@hageboeck hageboeck deleted the patch-1 branch January 20, 2025 16:55
devajithvs pushed a commit to devajithvs/llvm-project that referenced this pull request Jan 24, 2025
In gcc-15, explicit includes of `<cstdint>` are required when fixed-size
integers are used. In this file, this include only happened as a side
effect of including SmallVector.h

Although llvm compiles fine, the root-project would benefit from
explicitly including it here, so we can backport the patch.

(cherry picked from commit 7abf440)
devajithvs pushed a commit to devajithvs/llvm-project that referenced this pull request Feb 6, 2025
In gcc-15, explicit includes of `<cstdint>` are required when fixed-size
integers are used. In this file, this include only happened as a side
effect of including SmallVector.h

Although llvm compiles fine, the root-project would benefit from
explicitly including it here, so we can backport the patch.

(cherry picked from commit 7abf440)
devajithvs pushed a commit to devajithvs/llvm-project that referenced this pull request Feb 6, 2025
In gcc-15, explicit includes of `<cstdint>` are required when fixed-size
integers are used. In this file, this include only happened as a side
effect of including SmallVector.h

Although llvm compiles fine, the root-project would benefit from
explicitly including it here, so we can backport the patch.

(cherry picked from commit 7abf440)
devajithvs pushed a commit to devajithvs/llvm-project that referenced this pull request Feb 7, 2025
In gcc-15, explicit includes of `<cstdint>` are required when fixed-size
integers are used. In this file, this include only happened as a side
effect of including SmallVector.h

Although llvm compiles fine, the root-project would benefit from
explicitly including it here, so we can backport the patch.

Maybe interesting for @hahnjo and @vgvassilev
giordano pushed a commit to JuliaLang/llvm-project that referenced this pull request May 21, 2025
In gcc-15, explicit includes of `<cstdint>` are required when fixed-size
integers are used. In this file, this include only happened as a side
effect of including SmallVector.h

Although llvm compiles fine, the root-project would benefit from
explicitly including it here, so we can backport the patch.

Maybe interesting for @hahnjo and @vgvassilev

(cherry picked from commit 7abf440)
giordano pushed a commit to JuliaLang/llvm-project that referenced this pull request May 21, 2025
In gcc-15, explicit includes of `<cstdint>` are required when fixed-size
integers are used. In this file, this include only happened as a side
effect of including SmallVector.h

Although llvm compiles fine, the root-project would benefit from
explicitly including it here, so we can backport the patch.

Maybe interesting for @hahnjo and @vgvassilev

(cherry picked from commit 7abf440)
giordano pushed a commit to JuliaLang/llvm-project that referenced this pull request May 21, 2025
In gcc-15, explicit includes of `<cstdint>` are required when fixed-size
integers are used. In this file, this include only happened as a side
effect of including SmallVector.h

Although llvm compiles fine, the root-project would benefit from
explicitly including it here, so we can backport the patch.

Maybe interesting for @hahnjo and @vgvassilev

(cherry picked from commit 7abf440)
giordano pushed a commit to JuliaLang/llvm-project that referenced this pull request May 21, 2025
In gcc-15, explicit includes of `<cstdint>` are required when fixed-size
integers are used. In this file, this include only happened as a side
effect of including SmallVector.h

Although llvm compiles fine, the root-project would benefit from
explicitly including it here, so we can backport the patch.

Maybe interesting for @hahnjo and @vgvassilev

(cherry picked from commit 7abf440)
giordano pushed a commit to JuliaLang/llvm-project that referenced this pull request May 21, 2025
In gcc-15, explicit includes of `<cstdint>` are required when fixed-size
integers are used. In this file, this include only happened as a side
effect of including SmallVector.h

Although llvm compiles fine, the root-project would benefit from
explicitly including it here, so we can backport the patch.

Maybe interesting for @hahnjo and @vgvassilev

(cherry picked from commit 7abf440)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants