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

[Louis Dionne via Phabricator via libcxx-commits]

ldionne 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:
> > > 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146294