[libcxx-commits] [PATCH] D146294: [libcxx] Fix crash in std::stringstream with payload >= INT_MAX

Wed May 17 12:45:57 PDT 2023

azat added inline comments.

================
Comment at: libcxx/test/std/input.output/string.streams/stringstream.members/gcount.pass.cpp:19
+int main(int, char**) {
+#if __SIZE_WIDTH__ == 64
+  std::stringstream ss;
----------------
ldionne wrote:
> Mordante wrote:
> > azat wrote:
> > > azat wrote:
> > > > ldionne wrote:
> > > > > azat wrote:
> > > > > > ldionne wrote:
> > > > > > > azat wrote:
> > > > > > > > azat wrote:
> > > > > > > > > ldionne wrote:
> > > > > > > > > > azat wrote:
> > > > > > > > > > > Mordante wrote:
> > > > > > > > > > > > azat wrote:
> > > > > > > > > > > > > Mordante wrote:
> > > > > > > > > > > > > > Why is this needed? Can't you just compare that an int is 32-bit?
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > I ideally I would rather make the test unsupported on platform that are not used. Otherwise this test gives a false sense of doing something.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Maybe we should add a feature in `libcxx/utils/libcxx/test/features.py` Something like `can-test-gcount` Then we only enable it on 64-bit platforms where int is a 32-bit value. WDYT?
> > > > > > > > > > > > > Yeah, I did not like it either, but that was simpler.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > I don't think that introducing special flag only for one test will be a good choice however.
> > > > > > > > > > > > > What do you think about something like this instead:
> > > > > > > > > > > > > 
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > // UNSUPPORTED: target={{.*}}32{{.*}}
> > > > > > > > > > > > > ```
> > > > > > > > > > > > > 
> > > > > > > > > > > > > But this will not work for some arches (not sure that they are added to CI), i.e. `x86`.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > So instead I could add a separate parameter `libcxx/utils/libcxx/test/params.py` something like `size_t_bytes`, although I'm not sure is there an easy way to extract this in python, maybe you have an idea?
> > > > > > > > > > > > The tests are also used outside libc++ so even when we don't test x86, other might test that platform.
> > > > > > > > > > > > 
> > > > > > > > > > > > I don't mind adding a feature for a few test test. We already have precedence for them. For example
> > > > > > > > > > > > 1 user host-has-gdb-with-python
> > > > > > > > > > > > 2 users for glibc-old-ru_RU-decimal-point
> > > > > > > > > > > > 2 users for win32-broken-printf-g-precision
> > > > > > > > > > > > 
> > > > > > > > > > > > I'm not well enough versed in Python to know whether that's even possible. Even if it is I'm not sure whether Python uses the same widths as C++. I can imagine Python always using 64-bit integrals.
> > > > > > > > > > > > 
> > > > > > > > > > > > I'm not against your suggestion as long as we can do it reliable.
> > > > > > > > > > > I've found more or less simple, added two new parameters - `libcxx-32-bits` and `libcxx-64-bits`, and used `UNSUPPORTED: libcxx-32-bits` in the test, and `llvm-lit` successfully skips it if `libcxx-32-bits` is set. What do you think about this?
> > > > > > > > > > Sorry for not chiming in earlier, but IMO there's a lot of added complexity by trying to introduce a new lit feature here. For example, we should not have to change `cmake-bridge.cfg.in` when adding most new tests.
> > > > > > > > > > 
> > > > > > > > > > Is there a reason why we don't simply mark the test as `UNSUPPORTED` on the targets where it fails? There should be a limited number of those.
> > > > > > > > > The reason is simply: I'm not sure that I don't know all 32 bit targets that you have in CI
> > > > > > > > > 
> > > > > > > > > If someone can give me them, or a link where I can get them, I will replace this new feature with just a list instead.
> > > > > > > > > I thought about using *32* but AFAIR it does not cover all the targets.
> > > > > > > > Also, I don't know that is the typical use case of the llvm/clang developer, but I guess this feature 
> > > > > > > Try removing `UNSUPPORTED: libcxx-32-bits`, then re-upload the patch and watch the CI failures. Then you can add targets to the failure list per the CI failures.
> > > > > > But AFAIR not all tests will be run for this patch, more various configurations will be run for the main branch? (I remember this from some previous patch, for which CI did not shows the problem, but them CI from main did show)
> > > > > All the configurations we support should be tested in the pre-commit CI. If this is checked in with our CI green and someone complains about another bot somewhere being broken, that's another discussion we need to have with them, but it shouldn't impact your ability to land this. At least that's the way we try to have things, otherwise it's really impossible to know when it's safe to land something.
> > > > Ok, will replace this new parameter with simply `UNSUPPORTED`
> > > > 
> > > > >All the configurations we support should be tested in the pre-commit CI. If this is checked in with our CI green and someone complains about another bot somewhere being broken, that's another discussion we need to have with them, but it shouldn't impact your ability to land this. At least that's the way we try to have things, otherwise it's really impossible to know when it's safe to land something.
> > > > 
> > > > Here is one example - https://reviews.llvm.org/D133841#3789855
> > > >Is there a reason why we don't simply mark the test as UNSUPPORTED on the targets where it fails? There should be a limited number of those.
> > > 
> > > Ok, done (got all failures from one of the previous CI attempts - https://reviews.llvm.org/harbormaster/unit/220047/)
> > > Sorry for not chiming in earlier, but IMO there's a lot of added complexity by trying to introduce a new lit feature here. For example, we should not have to change `cmake-bridge.cfg.in` when adding most new tests.
> > > 
> > > Is there a reason why we don't simply mark the test as `UNSUPPORTED` on the targets where it fails? There should be a limited number of those.
> > 
> > @ldionne what's wrong with adding a lit feature for 32-bit? I agree it should no be part of `cmake-bridge.cfg.in`. A simple compile test with a `static_asssert` would work. As pointed out we have precedence to add features used in a few tests.
> > what's wrong with adding a lit feature for 32-bit?
> 
> Nothing, actually. It's just that my preference would be to use the target triple since we have that mechanism. We used to have a 32 bit Lit configuration (and an associated CI bot), and it was removed in favour of testing specific target triples. The precedent I was going for here was to do roughly what we did in `libcxx/test/libcxx/strings/basic.string/string.capacity/max_size.pass.cpp`, except using Lit features instead of `#ifdef`.
> 
> > As pointed out we have precedence to add features used in a few tests.
> 
> That's true, however those are often things that we literally can't test without running some significant runtime code (e.g. `win32-broken-printf-g-precision`).
> 
> TLDR I think a feature would be fine too and it's OK to add new Lit features (it's meant for that!), but in this case it would be used in a single test and using the target triple does as good a job, so that was the motivation behind my comment.
>All the configurations we support should be tested in the pre-commit CI

@ldionne apparently it is not true, I've just received an email that the test fails on `llvm-clang-win-x-armv7l`, I can submit one more `UNSUPPORTED: *armv7l*` to fix this, or a more generic fix.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146294/new/

https://reviews.llvm.org/D146294