Allow different cflags depending on extensions in system_libs.py#26037
Allow different cflags depending on extensions in system_libs.py#26037aheejin wants to merge 10 commits intoemscripten-core:mainfrom
Conversation
We cannot give different cflags depending on the file extensions in the current `system_libs.py`. This can be necessary when a library contains files with multiple extesnsions and some flags only work on a single extension. For example, `-std=c23` only works on .c files, while `-std=c++23` only on .cpp files. This adds a `filename` argument to the system library builder, so that we later can allow different cflags for different extensions.
Yes, I think this approach is probably better so we match upstream flags. |
sbc100
left a comment
There was a problem hiding this comment.
Does the filename argument need to be optional?
| cmd += get_base_cflags(self.build_dir, preprocess=False) | ||
| else: | ||
| cmd += cflags | ||
| cmd += self.get_cflags(src) |
There was a problem hiding this comment.
Since we already have the extension calculated here perhaps we can add self.cxxflags here when we have a cxx extension?
There was a problem hiding this comment.
Not sure if I understand... We don't have self.cflags either. Do you want to add both? But then why would we have both self.cflags and self.get_cflags() then?
There was a problem hiding this comment.
We do have self.cflags and the default self.get_cflags() will read from this field.
The idea is that for simple libraries where the cflags don't vary you can just do cflags = ['x', 'y', 'z'] at the class level.
See libunwind for example.
In this case we could just add support for cxxflags = ['x', 'y', 'z']?
There was a problem hiding this comment.
I tried adding get_cxxflags method here: b482af8
Currently get_cxxflags is set to call get_cflags by default and you can override it.
This doesn't add self.cxxflags because I think then we need to duplicate functions like this for cxxflags:
emscripten/tools/system_libs.py
Lines 589 to 601 in 69334c6
No I don't think so. Changed to a normal argument. |
This adds a test that we compile .c and .cpp files in libunwind with different cflags: .c with `-std=c23`, .cpp with `-std=c++23`.
| return self._get_common_flags() + ['-std=c23'] | ||
|
|
||
| def get_cxxflags(self): | ||
| return self._get_common_flags() + ['-std=c++23'] |
There was a problem hiding this comment.
I was just looking at libunwin upstream and it seems they do not use c23/c++23, but C++17:
Do I don't think we want to do this maybe? Since we don't want to differ from upstream here.
sbc100
left a comment
There was a problem hiding this comment.
lgtm, although I don't know if we actually want to do this for libunwind in particular.
|
I proposed a revert of llvm/llvm-project#125412. In the mean time can we instead use |
I think we add |
Yeah that's what I originally did... 🙃🫠😞 |
We cannot give different cflags depending on the file extensions in the current
system_libs.py. This can be necessary when a library contains files with multiple extesnsions and some flags only work on a single extension. For example,-std=c23only works on .c files, while-std=c++23only on .cpp files.This adds a
filenameargument to the system library builder, so that we later can allow different cflags for different extensions. This also handlesUSE_NINJApath.A test for libunwind is added that we compile .c files with
-std=c23and .cpp files with-std=c++23. (This we want to do for LLVM 21 anyway)