add test for inlinealways function attribute #426

folkertdev · 2023-06-26T21:58:11Z

Description

Improve test for attributes on functions. Attributes on the Function location were not tested at all. The alignstack attribute was applied to the return type, but this attribute is not actually valid in that position. A verify() on the module called this issue out.

Checklist

I have read the Contributing Guide

folkertdev · 2023-06-26T21:58:57Z

src/attributes.rs

+    // private function to make code elsewhere easier
+    #[llvm_versions(4.0..12.0)]
+    fn is_type(self) -> bool {
+        false
+    }


this feels a bit cheaty, but it's the simplest way I saw to keep code duplication to a minimum. Suggestions are welcome.

folkertdev · 2023-06-26T22:00:57Z

tests/all/test_attributes.rs

-    let alignstack_attribute = Attribute::get_named_enum_kind_id("alignstack");
-    let enum_attribute = context.create_enum_attribute(alignstack_attribute, 1);


alignstack is not a valid attibute on a function return type. At least for llvm 15, any value beside 0 (1 on line 88 here) leads to issues. The attribute is still applied when that value is 0. I think the value is only useful for some attributes (e.g. dereferencable(n), where n is the enum value).

to apply the noalias attribute instead, the function return type must be a pointer type.

folkertdev · 2023-06-26T22:02:07Z

src/attributes.rs

 pub struct Attribute {
    pub(crate) attribute: LLVMAttributeRef,
 }

+impl std::fmt::Debug for Attribute {


I used the Debug and PartialEq instance during debugging. I can remove them again, but then when something fails you only see an address, which is not helpful at all.

folkertdev · 2023-06-26T22:04:12Z

is CI supposed to work for LLVM 16? it seems to fail before it even tries to build/run tests, like LLVM 16 is just not installed on the CI machine.

TheDan64 · 2023-06-27T14:25:53Z

Yes, 16 was working before. Not sure what changed

folkertdev · 2023-06-27T19:11:09Z

well I'm not sure what changed then. The branch is rebased on current TheDan64:master so it should be using whatever master is using.

TheDan64 · 2023-06-30T04:56:27Z

@FawazTirmizi do you think you'd be able to look into LLVM16 suddenly failing in CI?

folkertdev · 2023-07-14T13:02:44Z

gently bumping this

TheDan64 · 2023-07-14T13:23:13Z

Hi, sorry for the delay. The llvm16 build will have to get fixed before this can be merged

folkertdev · 2023-07-14T13:33:24Z

yes. Anything I can help with for that? the failure seems kind of random. Maybe we should just retry it?

TheDan64 · 2023-07-14T22:19:43Z

Yeah, I'll rerun it but I'm not confident it'll fix it

TheDan64 · 2023-07-21T01:42:16Z

Been looking into this but it's not really clear why it fails for the latest version but not the others..

TheDan64 · 2023-07-21T01:42:55Z

Been wondering if llvm-sys changed something in 160

folkertdev · 2023-07-21T14:57:27Z

I added a call to llvm-config to the CI script, and it prints

Run llvm-config --version --bindir --libdir
llvm-config: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.33' not found (required by llvm-config)
llvm-config: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.34' not found (required by llvm-config)
llvm-config: /lib/x86_64-linux-gnu/libc.so.6: version `GLIBC_2.32' not found (required by llvm-config)
llvm-config: /lib/x86_64-linux-gnu/libstdc++.so.6: version `GLIBCXX_3.4.29' not found (required by llvm-config)

that makes sense actually, the LLVM install action installs clang+llvm-16.0.4-x86_64-linux-gnu-ubuntu-22.04.tar.xz. So I think the VM that we run this on uses a version of ubuntu that is too old for the LLVM build that is used.

folkertdev · 2023-07-21T15:17:51Z

and it just looks like for llvm 16.04 ubuntu 22.04 is the only one for which a pre-built tar exists. For earlier point releases it was ubuntu 20.04 still.

I'm not sure what the best approach is here. I suspect we can configure this in github CI using something like this maybe

+        # only use ubuntu-22.04 for llvm 16 and higher
+        include:
+          - os: ubuntu-22.04
+            llvm-version: ["16.0", "16-0"]
+        exclude:
+          - os: ubuntu-20.04
+            llvm-version: ["16.0", "16-0"]

not pretty, but might do the trick. I have no idea if this is just a fluke or if llvm (in a point release ?!) decided do bump its minimum supported ubuntu version?

an alternative is to force an earlier version of llvm. 16.01 is the latest one that has a 20.04 build

TheDan64 · 2023-07-21T21:40:20Z

ugh, that's really annoying if they bumped the ubuntu version in a point release of all times

folkertdev · 2023-07-21T21:45:32Z

for point releases, the builds they provide are really spotty. The later point releases have no binaries for ubuntu at all...

so what is the best option here

use 22.04 for llvm 16 only (I assume we want to keep testing with an older OS where possible)
pin the llvm 16 version to 16.0.1, so ubuntu 20.04 can be used for everyone

TheDan64 · 2023-07-21T21:47:05Z

If all the older versions work on u22 then we should just update to that. I'm assuming that isn't the case, so we probably want to make newer versions start using u22 but keep the older ones on u20

folkertdev · 2023-07-21T22:25:51Z

allright, got it to work. Turns out there is a way to do this sort of thing compactly

TheDan64 · 2023-07-22T03:08:01Z

Oh wow, thank you for looking into it!

TheDan64

Thank you!

add test for inlinealways function attribute

92023e3

folkertdev commented Jun 26, 2023

View reviewed changes

TheDan64 assigned folkertdev Jun 27, 2023

have llvm-config print some things

fd5de2e

bump ubuntu version of CI

30072e2

folkertdev force-pushed the function-attribute-test branch from cc00832 to 73d19bc Compare July 21, 2023 15:18

folkertdev force-pushed the function-attribute-test branch from 73d19bc to 74cbef2 Compare July 21, 2023 22:15

try to use more recent ubuntu for llvm 16

9e13d16

folkertdev force-pushed the function-attribute-test branch from 74cbef2 to 9e13d16 Compare July 21, 2023 22:22

TheDan64 approved these changes Jul 22, 2023

View reviewed changes

TheDan64 merged commit eb3ca22 into TheDan64:master Jul 22, 2023
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add test for inlinealways function attribute #426

add test for inlinealways function attribute #426

folkertdev commented Jun 26, 2023

folkertdev Jun 26, 2023

folkertdev Jun 26, 2023

folkertdev Jun 26, 2023

folkertdev commented Jun 26, 2023

TheDan64 commented Jun 27, 2023

folkertdev commented Jun 27, 2023

TheDan64 commented Jun 30, 2023

folkertdev commented Jul 14, 2023

TheDan64 commented Jul 14, 2023

folkertdev commented Jul 14, 2023

TheDan64 commented Jul 14, 2023

TheDan64 commented Jul 21, 2023

TheDan64 commented Jul 21, 2023

folkertdev commented Jul 21, 2023

folkertdev commented Jul 21, 2023

TheDan64 commented Jul 21, 2023

folkertdev commented Jul 21, 2023

TheDan64 commented Jul 21, 2023

folkertdev commented Jul 21, 2023

TheDan64 commented Jul 22, 2023

TheDan64 left a comment

		let alignstack_attribute = Attribute::get_named_enum_kind_id("alignstack");
		let enum_attribute = context.create_enum_attribute(alignstack_attribute, 1);

add test for inlinealways function attribute #426

add test for inlinealways function attribute #426

Conversation

folkertdev commented Jun 26, 2023

Description

Checklist

folkertdev Jun 26, 2023

Choose a reason for hiding this comment

folkertdev Jun 26, 2023

Choose a reason for hiding this comment

folkertdev Jun 26, 2023

Choose a reason for hiding this comment

folkertdev commented Jun 26, 2023

TheDan64 commented Jun 27, 2023

folkertdev commented Jun 27, 2023

TheDan64 commented Jun 30, 2023

folkertdev commented Jul 14, 2023

TheDan64 commented Jul 14, 2023

folkertdev commented Jul 14, 2023

TheDan64 commented Jul 14, 2023

TheDan64 commented Jul 21, 2023

TheDan64 commented Jul 21, 2023

folkertdev commented Jul 21, 2023

folkertdev commented Jul 21, 2023

TheDan64 commented Jul 21, 2023

folkertdev commented Jul 21, 2023

TheDan64 commented Jul 21, 2023

folkertdev commented Jul 21, 2023

TheDan64 commented Jul 22, 2023

TheDan64 left a comment

Choose a reason for hiding this comment