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

Wed May 10 10:53:24 PDT 2023

azat marked 3 inline comments as done.
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;
----------------
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/)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146294